103728 2012-11-30 02:56:53 -0800 Optimize ColorMatrix filter 2012-12-05 02:38:11 -0800 1 1 1 Unclassified WebKit SVG 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 zherczeg webkit-unassigned dino krit webkit.review.bot zimmermann oldest_to_newest 779972 0 zherczeg 2012-11-30 02:56:53 -0800 ColorMatrix recalculates a lot of values in every pixels. 779979 1 176934 zherczeg 2012-11-30 03:06:03 -0800 Created attachment 176934 patch 780087 2 zherczeg 2012-11-30 07:07:25 -0800 The strange thing is that the result is not claped between [0..1]. And I couldn't find anything in the specification what happens if the result is > 1 or < 0. Anyway, this is a really powerful filter. 782586 3 krit 2012-12-04 08:52:45 -0800 (In reply to comment #2) > The strange thing is that the result is not claped between [0..1]. And I couldn't find anything in the specification what happens if the result is > 1 or < 0. Anyway, this is a really powerful filter. We clapped in the past, but decided to remove it from the spec and follow Opera (which did not clamp). Does the your result clamp? 782592 4 176934 krit 2012-12-04 08:55:57 -0800 Comment on attachment 176934 patch View in context: https://bugs.webkit.org/attachment.cgi?id=176934&action=review Looks reasonable to me. r=me. I am just not sure about the following: > Source/WebCore/platform/graphics/filters/FEColorMatrix.cpp:136 > + if (filterType == FECOLORMATRIX_TYPE_SATURATE) { > + components[0] = (0.213 + 0.787 * values[0]); > + components[1] = (0.715 - 0.715 * values[0]); > + components[2] = (0.072 - 0.072 * values[0]); > + components[3] = (0.213 - 0.213 * values[0]); > + components[4] = (0.715 + 0.285 * values[0]); > + components[5] = (0.072 - 0.072 * values[0]); > + components[6] = (0.213 - 0.213 * values[0]); > + components[7] = (0.715 - 0.715 * values[0]); > + components[8] = (0.072 + 0.928 * values[0]); > + } else if (filterType == FECOLORMATRIX_TYPE_HUEROTATE) { > + float cosHue = cos(values[0] * piFloat / 180); > + float sinHue = sin(values[0] * piFloat / 180); > + components[0] = 0.213 + cosHue * 0.787 - sinHue * 0.213; > + components[1] = 0.715 - cosHue * 0.715 - sinHue * 0.715; > + components[2] = 0.072 - cosHue * 0.072 + sinHue * 0.928; > + components[3] = 0.213 - cosHue * 0.213 + sinHue * 0.143; > + components[4] = 0.715 + cosHue * 0.285 + sinHue * 0.140; > + components[5] = 0.072 - cosHue * 0.072 - sinHue * 0.283; > + components[6] = 0.213 - cosHue * 0.213 - sinHue * 0.787; > + components[7] = 0.715 - cosHue * 0.715 + sinHue * 0.715; > + components[8] = 0.072 + cosHue * 0.928 + sinHue * 0.072; > + } Does it make sense to move this in inline functions? It looks so strange here and makes it less readable. Please consider it before landing. 783571 5 zherczeg 2012-12-05 02:38:11 -0800 Thanks for the review! > Does it make sense to move this in inline functions? It looks so strange here and makes it less readable. Please consider it before landing. Yes, I did it. Patch landed as: http://trac.webkit.org/changeset/136661 176934 2012-11-30 03:06:03 -0800 2012-12-04 08:55:57 -0800 patch 0001-colormatrix.patch text/plain 6742 zherczeg RnJvbSA0Y2Y0YjhkYjI0ZjkzNTUzMTIzMjAwNmNhMGZjMzhlMzUxNDFkMzc0IE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBab2x0YW4gSGVyY3plZyA8emhlcmN6ZWdAd2Via2l0Lm9yZz4K RGF0ZTogRnJpLCAzMCBOb3YgMjAxMiAwMzowMToxMyAtMDgwMApTdWJqZWN0OiBbUEFUQ0hdIGNv bG9ybWF0cml4CgotLS0KIFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyAgICAgICAgICAgICAgICAg ICAgICAgICAgIHwgICAxOCArKysrKysKIC4uLi9wbGF0Zm9ybS9ncmFwaGljcy9maWx0ZXJzL0ZF Q29sb3JNYXRyaXguY3BwICAgIHwgICA2NSArKysrKysrKysrKy0tLS0tLS0tLQogMiBmaWxlcyBj aGFuZ2VkLCA1MyBpbnNlcnRpb25zKCspLCAzMCBkZWxldGlvbnMoLSkKCmRpZmYgLS1naXQgYS9T b3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXgg M2RlNzlmOC4uYmJlYmU2YSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisr KyBiL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIxIEBACisyMDEyLTExLTMw ICBab2x0YW4gSGVyY3plZyAgPHpoZXJjemVnQHdlYmtpdC5vcmc+CisKKyAgICAgICAgT3B0aW1p emUgQ29sb3JNYXRyaXggZmlsdGVyCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3No b3dfYnVnLmNnaT9pZD0xMDM3MjgKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMh KS4KKworICAgICAgICBDb2xvck1hdHJpeCBmaWx0ZXIgcmVjYWxjdWxhdGVzIHNldmVyYWwgY29u c3RhbnRzIGZvcgorICAgICAgICBldmVyeSBwaXhlbC4gSXQgaXMgZW5vdWdoIHRvIGRvIHRoZXNl IGNhbGN1bGF0aW9ucyBvbmx5CisgICAgICAgIG9uY2UgZHVyaW5nIHRoZSBpbml0aWFsaXphdGlv bi4gU3R5bGUgaXNzdWVzIGFsc28gZml4ZWQuCisKKyAgICAgICAgRXhpc3RpbmcgdGVzdHMgY292 ZXIgdGhpcyBmZWF0dXJlLgorCisgICAgICAgICogcGxhdGZvcm0vZ3JhcGhpY3MvZmlsdGVycy9G RUNvbG9yTWF0cml4LmNwcDoKKyAgICAgICAgKFdlYkNvcmU6Om1hdHJpeCk6CisgICAgICAgIChX ZWJDb3JlOjpzYXR1cmF0ZUFuZEh1ZVJvdGF0ZSk6CisgICAgICAgIChXZWJDb3JlOjplZmZlY3RU eXBlKToKKwogMjAxMi0xMS0yNyAgRGFuIENhcm5leSAgPGRjYXJuZXlAZ29vZ2xlLmNvbT4KIAog ICAgICAgICBbVjhdIGFkZCBjb250ZXh0Rm9yV29ybGQgaGVscGVyIGZ1bmN0aW9uIHRvIFNjcmlw dENvbnRyb2xsZXIKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNz L2ZpbHRlcnMvRkVDb2xvck1hdHJpeC5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFw aGljcy9maWx0ZXJzL0ZFQ29sb3JNYXRyaXguY3BwCmluZGV4IDI1YjA4N2UuLmUxMTc4YmMgMTAw NjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL2ZpbHRlcnMvRkVDb2xv ck1hdHJpeC5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvZmlsdGVy cy9GRUNvbG9yTWF0cml4LmNwcApAQCAtNzUsOSArNzUsOSBAQCBib29sIEZFQ29sb3JNYXRyaXg6 OnNldFZhbHVlcyhjb25zdCBWZWN0b3I8ZmxvYXQ+ICZ2YWx1ZXMpCiAKIGlubGluZSB2b2lkIG1h dHJpeChmbG9hdCYgcmVkLCBmbG9hdCYgZ3JlZW4sIGZsb2F0JiBibHVlLCBmbG9hdCYgYWxwaGEs IGNvbnN0IFZlY3RvcjxmbG9hdD4mIHZhbHVlcykKIHsKLSAgICBmbG9hdCByID0gdmFsdWVzWzBd ICAqIHJlZCArIHZhbHVlc1sxXSAgKiBncmVlbiArIHZhbHVlc1syXSAgKiBibHVlICsgdmFsdWVz WzNdICAqIGFscGhhICsgdmFsdWVzWzRdICogMjU1OwotICAgIGZsb2F0IGcgPSB2YWx1ZXNbNV0g ICogcmVkICsgdmFsdWVzWzZdICAqIGdyZWVuICsgdmFsdWVzWzddICAqIGJsdWUgKyB2YWx1ZXNb OF0gICogYWxwaGEgKyB2YWx1ZXNbOV0gKiAyNTU7Ci0gICAgZmxvYXQgYiA9IHZhbHVlc1sxMF0g ICogcmVkICsgdmFsdWVzWzExXSAgKiBncmVlbiArIHZhbHVlc1sxMl0gKiBibHVlICsgdmFsdWVz WzEzXSAqIGFscGhhICsgdmFsdWVzWzE0XSAqIDI1NTsKKyAgICBmbG9hdCByID0gdmFsdWVzWzBd ICogcmVkICsgdmFsdWVzWzFdICogZ3JlZW4gKyB2YWx1ZXNbMl0gKiBibHVlICsgdmFsdWVzWzNd ICogYWxwaGEgKyB2YWx1ZXNbNF0gKiAyNTU7CisgICAgZmxvYXQgZyA9IHZhbHVlc1s1XSAqIHJl ZCArIHZhbHVlc1s2XSAqIGdyZWVuICsgdmFsdWVzWzddICogYmx1ZSArIHZhbHVlc1s4XSAqIGFs cGhhICsgdmFsdWVzWzldICogMjU1OworICAgIGZsb2F0IGIgPSB2YWx1ZXNbMTBdICogcmVkICsg dmFsdWVzWzExXSAqIGdyZWVuICsgdmFsdWVzWzEyXSAqIGJsdWUgKyB2YWx1ZXNbMTNdICogYWxw aGEgKyB2YWx1ZXNbMTRdICogMjU1OwogICAgIGZsb2F0IGEgPSB2YWx1ZXNbMTVdICogcmVkICsg dmFsdWVzWzE2XSAqIGdyZWVuICsgdmFsdWVzWzE3XSAqIGJsdWUgKyB2YWx1ZXNbMThdICogYWxw aGEgKyB2YWx1ZXNbMTldICogMjU1OwogCiAgICAgcmVkID0gcjsKQEAgLTg2LDMwICs4NiwxMSBA QCBpbmxpbmUgdm9pZCBtYXRyaXgoZmxvYXQmIHJlZCwgZmxvYXQmIGdyZWVuLCBmbG9hdCYgYmx1 ZSwgZmxvYXQmIGFscGhhLCBjb25zdCBWZQogICAgIGFscGhhID0gYTsKIH0KIAotaW5saW5lIHZv aWQgc2F0dXJhdGUoZmxvYXQmIHJlZCwgZmxvYXQmIGdyZWVuLCBmbG9hdCYgYmx1ZSwgY29uc3Qg ZmxvYXQmIHMpCitpbmxpbmUgdm9pZCBzYXR1cmF0ZUFuZEh1ZVJvdGF0ZShmbG9hdCYgcmVkLCBm bG9hdCYgZ3JlZW4sIGZsb2F0JiBibHVlLCBjb25zdCBmbG9hdCogY29tcG9uZW50cykKIHsKLSAg ICBmbG9hdCByID0gKDAuMjEzICsgMC43ODcgKiBzKSAqIHJlZCArICgwLjcxNSAtIDAuNzE1ICog cykgKiBncmVlbiArICgwLjA3MiAtIDAuMDcyICogcykgKiBibHVlOwotICAgIGZsb2F0IGcgPSAo MC4yMTMgLSAwLjIxMyAqIHMpICogcmVkICsgKDAuNzE1ICsgMC4yODUgKiBzKSAqIGdyZWVuICsg KDAuMDcyIC0gMC4wNzIgKiBzKSAqIGJsdWU7Ci0gICAgZmxvYXQgYiA9ICgwLjIxMyAtIDAuMjEz ICogcykgKiByZWQgKyAoMC43MTUgLSAwLjcxNSAqIHMpICogZ3JlZW4gKyAoMC4wNzIgKyAwLjky OCAqIHMpICogYmx1ZTsKLQotICAgIHJlZCA9IHI7Ci0gICAgZ3JlZW4gPSBnOwotICAgIGJsdWUg PSBiOwotfQotCi1pbmxpbmUgdm9pZCBodWVyb3RhdGUoZmxvYXQmIHJlZCwgZmxvYXQmIGdyZWVu LCBmbG9hdCYgYmx1ZSwgY29uc3QgZmxvYXQmIGh1ZSkKLXsKLSAgICBmbG9hdCBjb3NIdWUgPSBj b3MoaHVlICogcGlEb3VibGUgLyAxODApOyAKLSAgICBmbG9hdCBzaW5IdWUgPSBzaW4oaHVlICog cGlEb3VibGUgLyAxODApOyAKLSAgICBmbG9hdCByID0gcmVkICAgKiAoMC4yMTMgKyBjb3NIdWUg KiAwLjc4NyAtIHNpbkh1ZSAqIDAuMjEzKSArCi0gICAgICAgICAgICAgICBncmVlbiAqICgwLjcx NSAtIGNvc0h1ZSAqIDAuNzE1IC0gc2luSHVlICogMC43MTUpICsKLSAgICAgICAgICAgICAgIGJs dWUgICogKDAuMDcyIC0gY29zSHVlICogMC4wNzIgKyBzaW5IdWUgKiAwLjkyOCk7Ci0gICAgZmxv YXQgZyA9IHJlZCAgICogKDAuMjEzIC0gY29zSHVlICogMC4yMTMgKyBzaW5IdWUgKiAwLjE0Mykg KwotICAgICAgICAgICAgICAgZ3JlZW4gKiAoMC43MTUgKyBjb3NIdWUgKiAwLjI4NSArIHNpbkh1 ZSAqIDAuMTQwKSArCi0gICAgICAgICAgICAgICBibHVlICAqICgwLjA3MiAtIGNvc0h1ZSAqIDAu MDcyIC0gc2luSHVlICogMC4yODMpOwotICAgIGZsb2F0IGIgPSByZWQgICAqICgwLjIxMyAtIGNv c0h1ZSAqIDAuMjEzIC0gc2luSHVlICogMC43ODcpICsKLSAgICAgICAgICAgICAgIGdyZWVuICog KDAuNzE1IC0gY29zSHVlICogMC43MTUgKyBzaW5IdWUgKiAwLjcxNSkgKwotICAgICAgICAgICAg ICAgYmx1ZSAgKiAoMC4wNzIgKyBjb3NIdWUgKiAwLjkyOCArIHNpbkh1ZSAqIDAuMDcyKTsKKyAg ICBmbG9hdCByID0gcmVkICogY29tcG9uZW50c1swXSArIGdyZWVuICogY29tcG9uZW50c1sxXSAr IGJsdWUgKiBjb21wb25lbnRzWzJdOworICAgIGZsb2F0IGcgPSByZWQgKiBjb21wb25lbnRzWzNd ICsgZ3JlZW4gKiBjb21wb25lbnRzWzRdICsgYmx1ZSAqIGNvbXBvbmVudHNbNV07CisgICAgZmxv YXQgYiA9IHJlZCAqIGNvbXBvbmVudHNbNl0gKyBncmVlbiAqIGNvbXBvbmVudHNbN10gKyBibHVl ICogY29tcG9uZW50c1s4XTsKIAogICAgIHJlZCA9IHI7CiAgICAgZ3JlZW4gPSBnOwpAQCAtMTI4 LDYgKzEwOSwzMiBAQCB0ZW1wbGF0ZTxDb2xvck1hdHJpeFR5cGUgZmlsdGVyVHlwZT4KIHZvaWQg ZWZmZWN0VHlwZShVaW50OENsYW1wZWRBcnJheSogcGl4ZWxBcnJheSwgY29uc3QgVmVjdG9yPGZs b2F0PiYgdmFsdWVzKQogewogICAgIHVuc2lnbmVkIHBpeGVsQXJyYXlMZW5ndGggPSBwaXhlbEFy cmF5LT5sZW5ndGgoKTsKKyAgICBmbG9hdCBjb21wb25lbnRzWzldOworCisgICAgaWYgKGZpbHRl clR5cGUgPT0gRkVDT0xPUk1BVFJJWF9UWVBFX1NBVFVSQVRFKSB7CisgICAgICAgIGNvbXBvbmVu dHNbMF0gPSAoMC4yMTMgKyAwLjc4NyAqIHZhbHVlc1swXSk7CisgICAgICAgIGNvbXBvbmVudHNb MV0gPSAoMC43MTUgLSAwLjcxNSAqIHZhbHVlc1swXSk7CisgICAgICAgIGNvbXBvbmVudHNbMl0g PSAoMC4wNzIgLSAwLjA3MiAqIHZhbHVlc1swXSk7CisgICAgICAgIGNvbXBvbmVudHNbM10gPSAo MC4yMTMgLSAwLjIxMyAqIHZhbHVlc1swXSk7CisgICAgICAgIGNvbXBvbmVudHNbNF0gPSAoMC43 MTUgKyAwLjI4NSAqIHZhbHVlc1swXSk7CisgICAgICAgIGNvbXBvbmVudHNbNV0gPSAoMC4wNzIg LSAwLjA3MiAqIHZhbHVlc1swXSk7CisgICAgICAgIGNvbXBvbmVudHNbNl0gPSAoMC4yMTMgLSAw LjIxMyAqIHZhbHVlc1swXSk7CisgICAgICAgIGNvbXBvbmVudHNbN10gPSAoMC43MTUgLSAwLjcx NSAqIHZhbHVlc1swXSk7CisgICAgICAgIGNvbXBvbmVudHNbOF0gPSAoMC4wNzIgKyAwLjkyOCAq IHZhbHVlc1swXSk7CisgICAgfSBlbHNlIGlmIChmaWx0ZXJUeXBlID09IEZFQ09MT1JNQVRSSVhf VFlQRV9IVUVST1RBVEUpIHsKKyAgICAgICAgZmxvYXQgY29zSHVlID0gY29zKHZhbHVlc1swXSAq IHBpRmxvYXQgLyAxODApOworICAgICAgICBmbG9hdCBzaW5IdWUgPSBzaW4odmFsdWVzWzBdICog cGlGbG9hdCAvIDE4MCk7CisgICAgICAgIGNvbXBvbmVudHNbMF0gPSAwLjIxMyArIGNvc0h1ZSAq IDAuNzg3IC0gc2luSHVlICogMC4yMTM7CisgICAgICAgIGNvbXBvbmVudHNbMV0gPSAwLjcxNSAt IGNvc0h1ZSAqIDAuNzE1IC0gc2luSHVlICogMC43MTU7CisgICAgICAgIGNvbXBvbmVudHNbMl0g PSAwLjA3MiAtIGNvc0h1ZSAqIDAuMDcyICsgc2luSHVlICogMC45Mjg7CisgICAgICAgIGNvbXBv bmVudHNbM10gPSAwLjIxMyAtIGNvc0h1ZSAqIDAuMjEzICsgc2luSHVlICogMC4xNDM7CisgICAg ICAgIGNvbXBvbmVudHNbNF0gPSAwLjcxNSArIGNvc0h1ZSAqIDAuMjg1ICsgc2luSHVlICogMC4x NDA7CisgICAgICAgIGNvbXBvbmVudHNbNV0gPSAwLjA3MiAtIGNvc0h1ZSAqIDAuMDcyIC0gc2lu SHVlICogMC4yODM7CisgICAgICAgIGNvbXBvbmVudHNbNl0gPSAwLjIxMyAtIGNvc0h1ZSAqIDAu MjEzIC0gc2luSHVlICogMC43ODc7CisgICAgICAgIGNvbXBvbmVudHNbN10gPSAwLjcxNSAtIGNv c0h1ZSAqIDAuNzE1ICsgc2luSHVlICogMC43MTU7CisgICAgICAgIGNvbXBvbmVudHNbOF0gPSAw LjA3MiArIGNvc0h1ZSAqIDAuOTI4ICsgc2luSHVlICogMC4wNzI7CisgICAgfQorCiAgICAgZm9y ICh1bnNpZ25lZCBwaXhlbEJ5dGVPZmZzZXQgPSAwOyBwaXhlbEJ5dGVPZmZzZXQgPCBwaXhlbEFy cmF5TGVuZ3RoOyBwaXhlbEJ5dGVPZmZzZXQgKz0gNCkgewogICAgICAgICBmbG9hdCByZWQgPSBw aXhlbEFycmF5LT5pdGVtKHBpeGVsQnl0ZU9mZnNldCk7CiAgICAgICAgIGZsb2F0IGdyZWVuID0g cGl4ZWxBcnJheS0+aXRlbShwaXhlbEJ5dGVPZmZzZXQgKyAxKTsKQEAgLTEzOCwxMSArMTQ1LDkg QEAgdm9pZCBlZmZlY3RUeXBlKFVpbnQ4Q2xhbXBlZEFycmF5KiBwaXhlbEFycmF5LCBjb25zdCBW ZWN0b3I8ZmxvYXQ+JiB2YWx1ZXMpCiAgICAgICAgICAgICBjYXNlIEZFQ09MT1JNQVRSSVhfVFlQ RV9NQVRSSVg6CiAgICAgICAgICAgICAgICAgbWF0cml4KHJlZCwgZ3JlZW4sIGJsdWUsIGFscGhh LCB2YWx1ZXMpOwogICAgICAgICAgICAgICAgIGJyZWFrOwotICAgICAgICAgICAgY2FzZSBGRUNP TE9STUFUUklYX1RZUEVfU0FUVVJBVEU6IAotICAgICAgICAgICAgICAgIHNhdHVyYXRlKHJlZCwg Z3JlZW4sIGJsdWUsIHZhbHVlc1swXSk7Ci0gICAgICAgICAgICAgICAgYnJlYWs7CisgICAgICAg ICAgICBjYXNlIEZFQ09MT1JNQVRSSVhfVFlQRV9TQVRVUkFURToKICAgICAgICAgICAgIGNhc2Ug RkVDT0xPUk1BVFJJWF9UWVBFX0hVRVJPVEFURToKLSAgICAgICAgICAgICAgICBodWVyb3RhdGUo cmVkLCBncmVlbiwgYmx1ZSwgdmFsdWVzWzBdKTsKKyAgICAgICAgICAgICAgICBzYXR1cmF0ZUFu ZEh1ZVJvdGF0ZShyZWQsIGdyZWVuLCBibHVlLCBjb21wb25lbnRzKTsKICAgICAgICAgICAgICAg ICBicmVhazsKICAgICAgICAgICAgIGNhc2UgRkVDT0xPUk1BVFJJWF9UWVBFX0xVTUlOQU5DRVRP QUxQSEE6CiAgICAgICAgICAgICAgICAgbHVtaW5hbmNlKHJlZCwgZ3JlZW4sIGJsdWUsIGFscGhh KTsKLS0gCjEuNy4yLjUKCg==