60757 2011-05-13 04:55:24 -0700 [CAIRO] Don't copy the surface before drawing it into the context in ShareableBitmap::paint() 2011-05-27 09:14:56 -0700 1 1 1 Unclassified WebKit WebKit2 528+ (Nightly build) PC Linux RESOLVED FIXED Cairo, Gtk P2 Normal --- 1 cgarcia webkit-unassigned mrobinson oldest_to_newest 403478 0 cgarcia 2011-05-13 04:55:24 -0700 cairo_image_surface_create_for_data() is already a shallow copy of the image, so we don't need to copy it again. 403479 1 93434 cgarcia 2011-05-13 04:58:47 -0700 Created attachment 93434 Patch 403569 2 mrobinson 2011-05-13 08:38:22 -0700 (In reply to comment #0) > cairo_image_surface_create_for_data() is already a shallow copy of the image, so we don't need to copy it again. Notice that in ShareableBitmapCG.cpp (the CG version of this file), the code does a copy of the image. Perhaps that this is just to support self copies though. If that's the case, it would be better to support self-copies by detecting when the target of the context is the same as the ShareableBitmap. Please contect the Apple team to see what the correct behavior is here. 403570 3 93434 mrobinson 2011-05-13 08:38:48 -0700 Comment on attachment 93434 Patch Removing review flag until we get some input from the authors of the CG version of ShareableBitmap. 403722 4 cgarcia 2011-05-13 11:58:06 -0700 (In reply to comment #2) > (In reply to comment #0) > > cairo_image_surface_create_for_data() is already a shallow copy of the image, so we don't need to copy it again. > > Notice that in ShareableBitmapCG.cpp (the CG version of this file), the code does a copy of the image. Perhaps that this is just to support self copies though. If that's the case, it would be better to support self-copies by detecting when the target of the context is the same as the ShareableBitmap. Please contect the Apple team to see what the correct behavior is here. But we are creating a new surface and copying that new surface, CG copies the already existing image, I think, which is the same than simply create a new surface with the image data. 403736 5 mrobinson 2011-05-13 12:19:43 -0700 (In reply to comment #4) > > Notice that in ShareableBitmapCG.cpp (the CG version of this file), the code does a copy of the image. Perhaps that this is just to support self copies though. If that's the case, it would be better to support self-copies by detecting when the target of the context is the same as the ShareableBitmap. Please contect the Apple team to see what the correct behavior is here. > > But we are creating a new surface and copying that new surface, CG copies the already existing image, I think, which is the same than simply create a new surface with the image data. The documentation for CGBitmapContextCreateImage states: The CGImage object returned by this function is created by a copy operation. Subsequent changes to the bitmap graphics context do not affect the contents of the returned image. In some cases the copy operation actually follows copy-on-write semantics, so that the actual physical copy of the bits occur only if the underlying data in the bitmap graphics context is modified. As a consequence, you may want to use the resulting image and release it before you perform additional drawing into the bitmap graphics context. In this way, you can avoid the actual physical copy of the data. In the case of Cairo, unless you explicitly create a copy of the original surface, all surfaces will be backed by the same block of data. Thus it seems the semantics of the blit could deviate if the ShareableBitmap is painting onto itself. In my opinion, we should check with the authors of the CG version. If the bitmap will never do a self-paint we can make your optimization, otherwise we can optimize only when the backing surface of the GraphicsContext does not contain the same bytes as the ShareableBitmap. 411478 6 cgarcia 2011-05-27 09:14:56 -0700 Committed r87517: <http://trac.webkit.org/changeset/87517> 93434 2011-05-13 04:58:47 -0700 2011-05-27 08:35:32 -0700 Patch wk2-shareable-bitmap-cairo.diff text/plain 1923 cgarcia ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQyL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQyL0No YW5nZUxvZwppbmRleCBkMDljYjNjLi5hNmZkOWVjIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0 Mi9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTYg QEAKKzIwMTEtMDUtMTMgIENhcmxvcyBHYXJjaWEgQ2FtcG9zICA8Y2dhcmNpYUBpZ2FsaWEuY29t PgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFtDQUlS T10gRG9uJ3QgY29weSB0aGUgc3VyZmFjZSBiZWZvcmUgZHJhd2luZyBpdCBpbnRvIHRoZSBjb250 ZXh0IGluIFNoYXJlYWJsZUJpdG1hcDo6cGFpbnQoKQorICAgICAgICBodHRwczovL2J1Z3Mud2Vi a2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NjA3NTcKKworICAgICAgICBjYWlyb19pbWFnZV9zdXJm YWNlX2NyZWF0ZV9mb3JfZGF0YSgpIGlzIGFscmVhZHkgYSBzaGFsbG93IGNvcHkgb2YKKyAgICAg ICAgdGhlIGltYWdlLCBzbyB3ZSBkb24ndCBuZWVkIHRvIGNvcHkgaXQgYWdhaW4uCisKKyAgICAg ICAgKiBTaGFyZWQvY2Fpcm8vU2hhcmVhYmxlQml0bWFwQ2Fpcm8uY3BwOgorICAgICAgICAoV2Vi S2l0OjpTaGFyZWFibGVCaXRtYXA6OnBhaW50KToKKwogMjAxMS0wNS0xMiAgUHJhdGlrIFNvbGFu a2kgIDxwc29sYW5raUBhcHBsZS5jb20+CiAKICAgICAgICAgUmV2aWV3ZWQgYnkgQW50dGkgS29p dmlzdG8uCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0Mi9TaGFyZWQvY2Fpcm8vU2hhcmVhYmxl Qml0bWFwQ2Fpcm8uY3BwIGIvU291cmNlL1dlYktpdDIvU2hhcmVkL2NhaXJvL1NoYXJlYWJsZUJp dG1hcENhaXJvLmNwcAppbmRleCAxMzNmNjJmLi44YWZmM2U5IDEwMDY0NAotLS0gYS9Tb3VyY2Uv V2ViS2l0Mi9TaGFyZWQvY2Fpcm8vU2hhcmVhYmxlQml0bWFwQ2Fpcm8uY3BwCisrKyBiL1NvdXJj ZS9XZWJLaXQyL1NoYXJlZC9jYWlyby9TaGFyZWFibGVCaXRtYXBDYWlyby5jcHAKQEAgLTUwLDEx ICs1MCw4IEBAIHZvaWQgU2hhcmVhYmxlQml0bWFwOjpwYWludChHcmFwaGljc0NvbnRleHQmIGNv bnRleHQsIGNvbnN0IEludFBvaW50JiBkc3RQb2ludCwKICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgQ0FJUk9fRk9STUFUX0FSR0IzMiwKICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgbV9zaXpl LndpZHRoKCksIG1fc2l6ZS5oZWlnaHQoKSwKICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgbV9z aXplLndpZHRoKCkgKiA0KSk7Ci0KLSAgICAvLyBUaGlzIGNvcHkgaXMgbm90IGNvcHktb24td3Jp dGUsIHNvIHRoaXMgaXMgcHJvYmFibHkgc3ViLW9wdGltYWwuCi0gICAgUmVmUHRyPGNhaXJvX3N1 cmZhY2VfdD4gc3VyZmFjZUNvcHkgPSBjb3B5Q2Fpcm9JbWFnZVN1cmZhY2Uoc3VyZmFjZS5nZXQo KSk7CiAgICAgRmxvYXRSZWN0IGRlc3RSZWN0KGRzdFBvaW50LCBzcmNSZWN0LnNpemUoKSk7Ci0g ICAgY29udGV4dC5wbGF0Zm9ybUNvbnRleHQoKS0+ZHJhd1N1cmZhY2VUb0NvbnRleHQoc3VyZmFj ZUNvcHkuZ2V0KCksIGRlc3RSZWN0LCBzcmNSZWN0LCAmY29udGV4dCk7CisgICAgY29udGV4dC5w bGF0Zm9ybUNvbnRleHQoKS0+ZHJhd1N1cmZhY2VUb0NvbnRleHQoc3VyZmFjZS5nZXQoKSwgZGVz dFJlY3QsIHNyY1JlY3QsICZjb250ZXh0KTsKIH0KIAogUGFzc1JlZlB0cjxjYWlyb19zdXJmYWNl X3Q+IFNoYXJlYWJsZUJpdG1hcDo6Y3JlYXRlQ2Fpcm9TdXJmYWNlKCkK