30333 2009-10-13 06:43:04 -0700 (Un)premultiplication code is not optimal and broken in alpha==0 case 2026-04-01 07:59:49 -0700 1 1 1 Unclassified WebKit SVG 528+ (Nightly build) PC Linux RESOLVED CONFIGURATION CHANGED P2 Normal --- 1 slomo webkit-unassigned ahmad.saleem792 bfulgham gustavo karlcow krit mdelaney7 oliver simon.fraser zalan oldest_to_newest 154324 0 slomo 2009-10-13 06:43:04 -0700 Hi, attached patch fixes the (Un)premultiplication code in WebCore/platform/graphics/Color.cpp. Explanation why it's broken follows 154325 1 41101 slomo 2009-10-13 06:50:58 -0700 Created attachment 41101 0002-Fix-Color-alpha-un-premultiplication-code.patch Now to the explanation. First of all, for consistency and correctness the resulting value of (un-)premultiplication with a 0-alpha should result in (0, 0, 0, 0) instead of the input value without changes. This is because: a) unpremultiplication: if input alpha is 0 all other components *must* be 0 too, otherwise the premultiplication was done incorrectly b) premultiplication: for non-0 alpha we multiply with alpha, if we do the same for 0 alpha the output will be (0, 0, 0, 0) as it should For the non-0 alpha cases it makes more sense to round to the nearest integer instead of always rounding upwards. Rounding upwards results in an uneven distribution of color values, i.e. a 0 component is much less likely than every other component value. 154341 2 41101 darin 2009-10-13 08:46:43 -0700 Comment on attachment 41101 0002-Fix-Color-alpha-un-premultiplication-code.patch Thanks for this contribution! The change seems OK, but we need a test case that demonstrates the effects this has. We don't take bug fixes without test cases. So please add a test case demonstrating the effect this has. One way to create a test case may be to use the <canvas> element, because that supports reading back pixel values from the canvas buffer. LayoutTests/fast/canvas/script-tests/set-colors.js is an example of a test case that might be adaptable to test this behavior. An alternative would be to explain why no test case is possible. That seems unlikely, though. Also, you need to run all existing tests and generate new results for any tests that now will have different results. review- because there is no test case. 154533 3 slomo 2009-10-13 21:47:49 -0700 (In reply to comment #2) > (From update of attachment 41101 [details]) > Thanks for this contribution! > > The change seems OK, but we need a test case that demonstrates the effects this > has. We don't take bug fixes without test cases. So please add a test case > demonstrating the effect this has. One way to create a test case may be to use > the <canvas> element, because that supports reading back pixel values from the > canvas buffer. > > LayoutTests/fast/canvas/script-tests/set-colors.js is an example of a test case > that might be adaptable to test this behavior. I just noticed this while working on other code that needs alpha premultiplication. I have absolutely *no* idea where this code here is used and how. I could provide a small test case that produces different results with/without the patch by just calling those functions but no idea how to do it from a canvas element. > An alternative would be to explain why no test case is possible. That seems > unlikely, though. > > Also, you need to run all existing tests and generate new results for any tests > that now will have different results. How can I do that? 154545 4 slomo 2009-10-14 01:23:57 -0700 Ok, I know how to run the layout tests now but this is really taking far too long for such a simply patch. I'll try to write a small testcase for this later though. Also this code is only used by the cairo backend in a few places, here's the relevant code from cairo itself and as you can see it does the same. http://cgit.freedesktop.org/cairo/tree/src/cairo-png.c#n53 http://cgit.freedesktop.org/cairo/tree/src/cairo-png.c#n408 154812 5 krit 2009-10-15 09:42:37 -0700 (In reply to comment #4) > Ok, I know how to run the layout tests now but this is really taking far too > long for such a simply patch. I'll try to write a small testcase for this later > though. > > Also this code is only used by the cairo backend in a few places, here's the > relevant code from cairo itself and as you can see it does the same. > > http://cgit.freedesktop.org/cairo/tree/src/cairo-png.c#n53 > http://cgit.freedesktop.org/cairo/tree/src/cairo-png.c#n408 There are already some tests for getImageData and putImageData in LayoutTests/fast/canvas/ This both calls are mainly using this color-code. Tests that can help you to figure out if your code works are: * canvas-putImageData.html * canvas-getImageData.html But I should warn you. canvas-getImageData might still fail because of bug 22150 It would be a great improvement if some more steps pass on canvas-getImageData.html. 154816 6 slomo 2009-10-15 09:51:06 -0700 Thanks, I'll take those tests as starting point 154818 7 slomo 2009-10-15 09:52:28 -0700 After reading bug #22150 it could be that my patch improves the situation there... we'll see. 203342 8 krit 2010-03-24 00:03:31 -0700 Simon added a test case for SVG and premultiplied/not-premultiplied Colors. Maybe this patch should get rid of the hack in bug 34383 and use this SVG test as test case. 203451 9 simon.fraser 2010-03-24 09:48:50 -0700 Had to work around this issue in http://trac.webkit.org/changeset/54106 1053191 10 slomo 2014-12-07 10:48:37 -0800 What should we do with this? Note that if you consider merging this patch, that the mail address used there is not valid anymore. Simon? 1900934 11 ahmad.saleem792 2022-09-24 16:05:20 -0700 Is this still an issue? I tried to look for the code from the patch but couldn't manage to find "colorFromPremultipliedARGB" and it seems that it was deleted in this commit: https://github.com/WebKit/WebKit/commit/9b9b872709ed845f54ad8f5aa9257bd0212c455d If not needed, can we close this bug or if this is still an issue, can we get update explanation etc.? Thanks! 2195909 12 karlcow 2026-04-01 07:59:49 -0700 The original functions (colorFromPremultipliedARGB) were deleted and replaced. Modern WebCore uses Color/SRGBA types with proper alpha handling. The premultiplication code has been completely rewritten across PixelBufferConversion.cpp, FilterImage.cpp, etc 41101 2009-10-13 06:50:58 -0700 2010-06-11 10:29:19 -0700 0002-Fix-Color-alpha-un-premultiplication-code.patch 0002-Fix-Color-alpha-un-premultiplication-code.patch text/plain 3626 slomo RnJvbSA3MmMxMjlkYTk0MmQ2ZmEyNzY4ZjdjYzZkMTc5MDg4YjAwYzZhNTkwIE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiA9P1VURi04P3E/U2ViYXN0aWFuPTIwRHI9QzM9QjZnZT89IDxz ZWJhc3RpYW4uZHJvZWdlQGNvbGxhYm9yYS5jby51az4KRGF0ZTogVHVlLCAxMyBPY3QgMjAwOSAx NTo0NjoyMSArMDIwMApTdWJqZWN0OiBbUEFUQ0ggMi8yXSBGaXggQ29sb3IgYWxwaGEgKHVuLSlw cmVtdWx0aXBsaWNhdGlvbiBjb2RlLgoKICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9z aG93X2J1Zy5jZ2k/aWQ9MzAzMzMKCiAgICAgICAgKiBwbGF0Zm9ybS9ncmFwaGljcy9Db2xvci5j cHA6CiAgICAgICAgKFdlYkNvcmU6OmNvbG9yRnJvbVByZW11bHRpcGxpZWRBUkdCKToKICAgICAg ICAoV2ViQ29yZTo6cHJlbXVsdGlwbGllZEFSR0JGcm9tQ29sb3IpOgogICAgICAgIEZpcnN0IG9m IGFsbCwgaWYgYWxwaGEgaXMgMCB0aGUgY29sb3IgaXMgMTAwJSB0cmFuc3BhcmVudAogICAgICAg IGFuZCAodW4tKXByZW11bHRpcGxpY2F0aW9uIHNob3VsZCByZXN1bHQgaW4gMC4KICAgICAgICBG b3IgdGhlIG5vbi0wIGFscGhhIGNhc2VzIGl0IG1ha2VzIG1vcmUgc2Vuc2UgdG8gcm91bmQgdG8K ICAgICAgICB0aGUgbmVhcmVzdCBpbnRlZ2VyIGluc3RlYWQgb2YgYWx3YXlzIHJvdW5kaW5nIHVw d2FyZHMuCiAgICAgICAgUm91bmRpbmcgdXB3YXJkcyByZXN1bHRzIGluIGFuIHVuZXZlbiBkaXN0 cmlidXRpb24gb2YgY29sb3IKICAgICAgICB2YWx1ZXMsIGkuZS4gYSAwIGNvbXBvbmVudCBpcyBt dWNoIGxlc3MgbGlrZWx5IHRoYW4gZXZlcnkKLS0tCiBXZWJDb3JlL0NoYW5nZUxvZyAgICAgICAg ICAgICAgICAgICB8ICAgMTkgKysrKysrKysrKysrKysrKysrKwogV2ViQ29yZS9wbGF0Zm9ybS9n cmFwaGljcy9Db2xvci5jcHAgfCAgIDE2ICsrKysrKysrLS0tLS0tLS0KIDIgZmlsZXMgY2hhbmdl ZCwgMjcgaW5zZXJ0aW9ucygrKSwgOCBkZWxldGlvbnMoLSkKCmRpZmYgLS1naXQgYS9XZWJDb3Jl L0NoYW5nZUxvZyBiL1dlYkNvcmUvQ2hhbmdlTG9nCmluZGV4IGYyNWMwMWIuLjJhYzlmODYgMTAw NjQ0Ci0tLSBhL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0x LDMgKzEsMjIgQEAKKzIwMDktMTAtMTMgIFNlYmFzdGlhbiBEcsO2Z2UgIDxzZWJhc3RpYW4uZHJv ZWdlQGNvbGxhYm9yYS5jby51az4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMh KS4KKworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MzAz MzMKKyAgICAgICAgCisgICAgICAgIEZpeCBDb2xvciBhbHBoYSAodW4tKXByZW11bHRpcGxpY2F0 aW9uIGNvZGUuCisKKyAgICAgICAgKiBwbGF0Zm9ybS9ncmFwaGljcy9Db2xvci5jcHA6CisgICAg ICAgIChXZWJDb3JlOjpjb2xvckZyb21QcmVtdWx0aXBsaWVkQVJHQik6CisgICAgICAgIChXZWJD b3JlOjpwcmVtdWx0aXBsaWVkQVJHQkZyb21Db2xvcik6CisgICAgICAgIEZpcnN0IG9mIGFsbCwg aWYgYWxwaGEgaXMgMCB0aGUgY29sb3IgaXMgMTAwJSB0cmFuc3BhcmVudAorICAgICAgICBhbmQg KHVuLSlwcmVtdWx0aXBsaWNhdGlvbiBzaG91bGQgcmVzdWx0IGluIDAuCisgICAgICAgIEZvciB0 aGUgbm9uLTAgYWxwaGEgY2FzZXMgaXQgbWFrZXMgbW9yZSBzZW5zZSB0byByb3VuZCB0bworICAg ICAgICB0aGUgbmVhcmVzdCBpbnRlZ2VyIGluc3RlYWQgb2YgYWx3YXlzIHJvdW5kaW5nIHVwd2Fy ZHMuCisgICAgICAgIFJvdW5kaW5nIHVwd2FyZHMgcmVzdWx0cyBpbiBhbiB1bmV2ZW4gZGlzdHJp YnV0aW9uIG9mIGNvbG9yCisgICAgICAgIHZhbHVlcywgaS5lLiBhIDAgY29tcG9uZW50IGlzIG11 Y2ggbGVzcyBsaWtlbHkgdGhhbiBldmVyeQorICAgICAgICBvdGhlciBjb21wb25lbnQgdmFsdWUu CisKIDIwMDktMTAtMTIgIEFsZXhhbmRlciBQYXZsb3YgIDxhcGF2bG92QGNocm9taXVtLm9yZz4K IAogICAgICAgICBSZXZpZXdlZCBieSBUaW1vdGh5IEhhdGNoZXIuCmRpZmYgLS1naXQgYS9XZWJD b3JlL3BsYXRmb3JtL2dyYXBoaWNzL0NvbG9yLmNwcCBiL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhp Y3MvQ29sb3IuY3BwCmluZGV4IGQ5OGIyMDIuLjkwMWE1MWYgMTAwNjQ0Ci0tLSBhL1dlYkNvcmUv cGxhdGZvcm0vZ3JhcGhpY3MvQ29sb3IuY3BwCisrKyBiL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhp Y3MvQ29sb3IuY3BwCkBAIC0zNjEsMTIgKzM2MSwxMiBAQCBDb2xvciBjb2xvckZyb21QcmVtdWx0 aXBsaWVkQVJHQih1bnNpZ25lZCBwaXhlbENvbG9yKQogICAgIFJHQkEzMiByZ2JhOwogCiAgICAg aWYgKHVuc2lnbmVkIGFscGhhID0gKHBpeGVsQ29sb3IgJiAweEZGMDAwMDAwKSA+PiAyNCkgewot ICAgICAgICByZ2JhID0gbWFrZVJHQkEoKChwaXhlbENvbG9yICYgMHgwMEZGMDAwMCkgPj4gMTYp ICogMjU1IC8gYWxwaGEsCi0gICAgICAgICAgICAgICAgICAgICAgICAoKHBpeGVsQ29sb3IgJiAw eDAwMDBGRjAwKSA+PiA4KSAqIDI1NSAvIGFscGhhLAotICAgICAgICAgICAgICAgICAgICAgICAg IChwaXhlbENvbG9yICYgMHgwMDAwMDBGRikgKiAyNTUgLyBhbHBoYSwKKyAgICAgICAgcmdiYSA9 IG1ha2VSR0JBKCgoKHBpeGVsQ29sb3IgJiAweDAwRkYwMDAwKSA+PiAxNikgKiAyNTUgKyBhbHBo YSAvIDIpIC8gYWxwaGEsCisgICAgICAgICAgICAgICAgICAgICAgICAoKChwaXhlbENvbG9yICYg MHgwMDAwRkYwMCkgPj4gOCkgKiAyNTUgKyBhbHBoYSAvIDIpIC8gYWxwaGEsCisgICAgICAgICAg ICAgICAgICAgICAgICAgKChwaXhlbENvbG9yICYgMHgwMDAwMDBGRikgKiAyNTUgKyBhbHBoYSAv IDIpIC8gYWxwaGEsCiAgICAgICAgICAgICAgICAgICAgICAgICAgIGFscGhhKTsKICAgICB9IGVs c2UKLSAgICAgICAgcmdiYSA9IHBpeGVsQ29sb3I7CisgICAgICAgIHJnYmEgPSBtYWtlUkdCQSgw LCAwLCAwLCAwKTsKIAogICAgIHJldHVybiBDb2xvcihyZ2JhKTsKIH0KQEAgLTM3NywxMSArMzc3 LDExIEBAIHVuc2lnbmVkIHByZW11bHRpcGxpZWRBUkdCRnJvbUNvbG9yKGNvbnN0IENvbG9yJiBj b2xvcikKIAogICAgIGlmICh1bnNpZ25lZCBhbHBoYSA9IGNvbG9yLmFscGhhKCkpIHsKICAgICAg ICAgcGl4ZWxDb2xvciA9IGFscGhhIDw8IDI0IHwKLSAgICAgICAgICAgICAoKGNvbG9yLnJlZCgp ICogYWxwaGEgICsgMjU0KSAvIDI1NSkgPDwgMTYgfCAKLSAgICAgICAgICAgICAoKGNvbG9yLmdy ZWVuKCkgKiBhbHBoYSAgKyAyNTQpIC8gMjU1KSA8PCA4IHwgCi0gICAgICAgICAgICAgKChjb2xv ci5ibHVlKCkgKiBhbHBoYSAgKyAyNTQpIC8gMjU1KTsKKyAgICAgICAgICAgICAoKGNvbG9yLnJl ZCgpICogYWxwaGEgKyAxMjgpIC8gMjU1KSA8PCAxNiB8IAorICAgICAgICAgICAgICgoY29sb3Iu Z3JlZW4oKSAqIGFscGhhICsgMTI4KSAvIDI1NSkgPDwgOCB8IAorICAgICAgICAgICAgICgoY29s b3IuYmx1ZSgpICogYWxwaGEgKyAxMjgpIC8gMjU1KTsKICAgICB9IGVsc2UKLSAgICAgICAgIHBp eGVsQ29sb3IgPSBjb2xvci5yZ2IoKTsKKyAgICAgICAgIHBpeGVsQ29sb3IgPSAwOwogCiAgICAg cmV0dXJuIHBpeGVsQ29sb3I7CiB9Ci0tIAoxLjYuNC4zCgo=