Bug 77245 – [Chromium] SVG Composite of Offset crashes

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Stephen Chenney]

Created attachment 124368 [details]
Test case, suitable for LayoutTest

What steps will reproduce the problem?
1. Load the attached test case as a chromium LayoutTest
2. In current r106139, it crashes
3. In r105663 it produces incorrect results

What is the expected output? What do you see instead?
You should see a green rectangle and an offset of it, with the upper portion of the image empty. In r105663, you see that the offset rectangle has been incorrectly clipped, and the difference operation that should have removed the upper rectangles has left something behind, because it is subtracting the clipped lower rectangle.

This works in Firefox, and is disabled in Safari.

[Stephen Chenney]

This bug may exist prior to r105663. It's a debug only assert.

[Stephen Chenney]

Somehow a pre-multiplied alpha image is being filled with non-zero RGB but zero alpha, a definite no-no.

[Stephen Chenney]

I have tracked down the crash to the arithmetic compositing behavior, where:

result = k1*i1*i2 + k2*i1 + k3*i2 + k4

with premultiplied alpha and k1 = k4 = 0, k2 = -1, k3 = 1. That is, computing a difference image in premultiplied alpha.

For opaque pixels, a = 255, this will always generate a = 0 result. But if the color values are not the same it will generate non-zero r, g, or b with a = 0 in a premultiplied pixel, which hits an assert in WebCore/platform/graphics/skia/SkiaUtils.cpp:91, in the method SkPMColorToColor.
The assert checks that r=g=b=0 when a=0, as is required for premultiplied alpha.

There are two possible fixes for this. One is to remove the assert and go with the behavior that a = 0 in a color conversion forces r=g=b=0 regardless of their values. This just makes the debug config behavior match the release behavior, and match other browsers.

The other is to add a test in WebCore before sending the pixel to Skia. I really don't want to do that, because it would add a branch in otherwise tight loops over all pixel locations.

So I'll put up a simple patch that removes the assert.

[Mike Reed]

I had assumed that "arithmetic" operated on unpremultiplied colors. Without that, we should expect to see lots and lots of illegal premul results. Where is this documented?

[Stephen White]

(In reply to comment #4)
> I had assumed that "arithmetic" operated on unpremultiplied colors. Without that, we should expect to see lots and lots of illegal premul results. Where is this documented?

"Unless otherwise stated, all image filters operate on premultiplied RGBA samples. Filters which work more naturally on non-premultiplied data (feColorMatrix and feComponentTransfer) will temporarily undo and redo premultiplication as specified. All raster effect filtering operations take 1 to N input RGBA images, additional attributes as parameters, and produce a single output RGBA image."

There doesn't seem to be an exception for feComposite with the "arithmetic" operator, but it seems like there should be.  We should probably do the unpremultiply/process/premultiply dance, but that doesn't seem to be in the spec.

[Stephen Chenney]

I'll come up with a test case we can use to verify the behavior of other browsers. We can then see about updating the spec or changing the current behavior.

[Mike Reed]

Is there an active discussion area somewhere to see what other people are thinking about arithmetic?

[Stephen Chenney]

Created attachment 124802 [details]
Test to verify behavior of arithmetic image ops

[Stephen Chenney]

The attached updated test case verifies that arithmetic ops are done pre-multiplied. I tested in Chrome and Firefox and got the same result.

I would expect this example, where result = constant, to produce the non-pre-multiplied answer in which you get a semi-opaque gray image rather than a semi-opaque white image.

[Stephen Chenney]

(In reply to comment #7)
> Is there an active discussion area somewhere to see what other people are thinking about arithmetic?

Not that I'm aware of. The place to start something would be the W3C SVG mailing list.

[Mike Reed]

Well, I can believe it is implemented wi/ premuls, I'm just not convinced that is correct/useful :) Have you tried IE9/10? If all 3 browsers agreed, and/or we can find some spec person somewhere to confirm this, I'll go ahead and change skia's implementation.

[Zoltan Herczeg]

(In reply to comment #4)
> I had assumed that "arithmetic" operated on unpremultiplied colors. Without that, we should expect to see lots and lots of illegal premul results. Where is this documented?

Yeah this behaviour is surprised me sometimes as well. But all browsers do this way.

[Stephen Chenney]

IE9 apparently doesn't support SVG filters, or at least not those I've used in the test cases. I'm setting up IE10 to test, but right now  believe that pre-multiplied is the specified way to do it and the way we should do it.

[Stephen White]

(In reply to comment #13)
> IE9 apparently doesn't support SVG filters, or at least not those I've used in the test cases. I'm setting up IE10 to test, but right now  believe that pre-multiplied is the specified way to do it and the way we should do it.

IE10PP3 and later (which also require Windows8, IIRC) support them.

[Mike Reed]

OK, we need to implement it in premul space (oiy). Either way, the impl must clamp the resulting values to keep them legal premul, or the images just won't compose correctly afterwards. The assert firing in Skia is not pedantic, it is signaling that this image will not draw as expected.

[Stephen Chenney]

Created attachment 125011 [details]
Patch

[WebKit Review Bot]

Attachment 125011 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

Traceback (most recent call last):
  File "Tools/Scripts/check-webkit-style", line 48, in <module>
    sys.exit(CheckWebKitStyle().main())
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/main.py", line 154, in main
    patch_checker.check(patch)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/patchreader.py", line 66, in check
    self._text_file_reader.process_file(file_path=path, line_numbers=line_numbers)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/filereader.py", line 130, in process_file
    self._processor.process(lines, file_path, **kwargs)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checker.py", line 826, in process
    checker.check(lines)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checkers/test_expectations.py", line 119, in check
    overrides=overrides)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checkers/test_expectations.py", line 96, in check_test_expectations
    is_lint_mode=True, overrides=overrides)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py", line 726, in __init__
    self._add_skipped_tests(port.skipped_tests(tests))
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/layout_tests/port/webkit.py", line 376, in skipped_tests
    return self.skipped_layout_tests(test_list)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/layout_tests/port/webkit.py", line 372, in skipped_layout_tests
    tests_to_skip.update(self._skipped_tests_for_unsupported_features(test_list))
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/layout_tests/port/webkit.py", line 319, in _skipped_tests_for_unsupported_features
    if self._has_test_in_directories(self._missing_feature_to_skipped_tests().values(), test_list):
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/layout_tests/port/webkit.py", line 311, in _has_test_in_directories
    for directory, test in itertools.product(directories, test_list):
TypeError: 'NoneType' object is not iterable


If any of these errors are false positives, please file a bug against check-webkit-style.

[Stephen Chenney]

OK folks, here's an attempt to fix the issue in the software arithmetic composite filter.

This causes our behavior to differ from Firefox, which seems to allow intermediate invalid pixel states. The change proposed here enforces that pixel states are always valid at every stage of a chained set of arithmetic composite operations.

To my mind we should take this patch, as invalid pixels can cause all sorts of problems as they propagate though the filter stack. It may also be a very little bit little faster (or a very little bit slower).

Feedback welcome.

[Mike Reed]

I agree that we should clamp the components against the computed alpha (and zero).

Having to copy/paste the same block of code 4 times (with no comment above each) makes it hard to scan for correctness. Can a local inline helper function encapsulate that block without incurring a perf hit?

I will change the skia impl to match this premul (instead of unpremul) and clamping behavior.

[Stephen Chenney]

Created attachment 125021 [details]
Patch

This removes the redundant code. I wanted to avoid static inlines to ensure that the template optimization happens (not sure how compilers will behave otherwise).

[WebKit Review Bot]

Attachment 125021 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

Traceback (most recent call last):
  File "Tools/Scripts/check-webkit-style", line 48, in <module>
    sys.exit(CheckWebKitStyle().main())
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/main.py", line 154, in main
    patch_checker.check(patch)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/patchreader.py", line 66, in check
    self._text_file_reader.process_file(file_path=path, line_numbers=line_numbers)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/filereader.py", line 130, in process_file
    self._processor.process(lines, file_path, **kwargs)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checker.py", line 826, in process
    checker.check(lines)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checkers/test_expectations.py", line 119, in check
    overrides=overrides)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checkers/test_expectations.py", line 96, in check_test_expectations
    is_lint_mode=True, overrides=overrides)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py", line 726, in __init__
    self._add_skipped_tests(port.skipped_tests(tests))
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/layout_tests/port/webkit.py", line 376, in skipped_tests
    return self.skipped_layout_tests(test_list)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/layout_tests/port/webkit.py", line 372, in skipped_layout_tests
    tests_to_skip.update(self._skipped_tests_for_unsupported_features(test_list))
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/layout_tests/port/webkit.py", line 319, in _skipped_tests_for_unsupported_features
    if self._has_test_in_directories(self._missing_feature_to_skipped_tests().values(), test_list):
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/layout_tests/port/webkit.py", line 311, in _has_test_in_directories
    for directory, test in itertools.product(directories, test_list):
TypeError: 'NoneType' object is not iterable


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nikolas Zimmermann]

(In reply to comment #14)
> (In reply to comment #13)
> > IE9 apparently doesn't support SVG filters, or at least not those I've used in the test cases. I'm setting up IE10 to test, but right now  believe that pre-multiplied is the specified way to do it and the way we should do it.
> 
> IE10PP3 and later (which also require Windows8, IIRC) support them.
I'd rather test with Opera, which has a stable SVG implementation for years now - especially for filters.

(In reply to comment #18)
> This causes our behavior to differ from Firefox, which seems to allow intermediate invalid pixel states. The change proposed here enforces that pixel states are always valid at every stage of a chained set of arithmetic composite operations.
We should create a test and try with Batik/Opera, FF. IE10 filter support is brand-new, likely to contain bugs as well - Batik/Opera implement this much longer.

> To my mind we should take this patch, as invalid pixels can cause all sorts of problems as they propagate though the filter stack. It may also be a very little bit little faster (or a very little bit slower).
Looking at your patch now, though I think you should first test against all viewers I mentioned, and if the outcome is different between most of them, then we should raise an issue with the SVG WG, to see how to proceed.

The history of SVG has proved, that we shouldn't do anything w/o agreeing, this will only lead to divergences.

[Nikolas Zimmermann]

Comment on attachment 125021 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=125021&action=review

First quick round of review, will not touch the r? state yet.

> LayoutTests/svg/filters/feComposite-arithmetic-invalid-rgba.svg:3
> +        <!-- This filter produces intermediate results that are invalid pre-multiplioed rgba pixels. -->

typo: pre-multiplied

> LayoutTests/svg/filters/feComposite-arithmetic-invalid-rgba.svg:6
> +        <!-- Specifically, after the 4th step an interior pixel will contain (0, 0.8, 0, 0.5) which  -->
> +        <!-- is invalid because g > a. When used in other operations, this may generate bad results, -->
> +        <!-- so we clamp the color values to [0, alpha] on all intermediate operations.              -->

This sounds wonderful, I hope we can take that approach.
CC'ing Zoltan & Dirk who should comment as well.

[Stephen Chenney]

Opera and Firefox clearly allow invalid intermediate results. That is, they have the same behavior that Webkit currently has (modulo the crash in Skia debug). Say we have this filter sequence:

<feComposite operator="arithmetic" in="SourceGraphic" in2="SourceGraphic" k1="0" k2="0.2" k3="0" k4="0" result="rgba02" />
<feComposite operator="arithmetic" in="SourceAlpha" in2="SourceAlpha" k1="0" k2="0.3" k3="0" k4="0" result="alpha05" />
<feComposite operator="arithmetic" in="rgba02" in2="alpha05" k1="0" k2="1" k3="1" k4="0" result="tmp" />
<feComposite operator="arithmetic" in="SourceGraphic" in2="tmp" k1="0" k2="1" k3="-1" k4="0" />
<feComposite operator="arithmetic" in="tmp" k1="0" k2="1" k3="1" k4="0" />

Given an input of (0, 255, 0, 255), the result is:

rgba02 contains (0, 51, 0, 51)
alpha05 contains (0, 0, 0, 77)
tmp contains (0, 51, 0, 128)
step 4 results in (0, 204, 0, 127) which is an invalid pre-mul rgba pixel
final result is (0, 255, 0, 255)

I think it's time to escalate to the SVG WG. The concrete proposal is to modify section 15.12 of the SVG 1.1 spec (16 August 2011) to change the statement:

"Additionally, a component-wise arithmetic operation (with the result clamped between [0..1]) can be applied."

to instead say

"Additionally, a component-wise arithmetic operation can be applied. Arithmetic results, including intermediate results, are clamped to alpha in [0,1] and color components in [0,alpha]."

On the example above, the sequence would be:

(0, 51, 0, 51)
(0, 0, 0, 77)
(0, 51, 0, 128)
(0, 127, 0, 127)
(0, 178, 0, 255)

As far as this bug goes, I've removed the review and commit flags. I'll look at alternate bug fixes that allow overflowing intermediate values.

[Nikolas Zimmermann]

(In reply to comment #24)
> Opera and Firefox clearly allow invalid intermediate results. That is, they have the same behavior that Webkit currently has (modulo the crash in Skia debug). Say we have this filter sequence:
> 
> <feComposite operator="arithmetic" in="SourceGraphic" in2="SourceGraphic" k1="0" k2="0.2" k3="0" k4="0" result="rgba02" />
> <feComposite operator="arithmetic" in="SourceAlpha" in2="SourceAlpha" k1="0" k2="0.3" k3="0" k4="0" result="alpha05" />
> <feComposite operator="arithmetic" in="rgba02" in2="alpha05" k1="0" k2="1" k3="1" k4="0" result="tmp" />
> <feComposite operator="arithmetic" in="SourceGraphic" in2="tmp" k1="0" k2="1" k3="-1" k4="0" />
> <feComposite operator="arithmetic" in="tmp" k1="0" k2="1" k3="1" k4="0" />
> 
> Given an input of (0, 255, 0, 255), the result is:
> 
> rgba02 contains (0, 51, 0, 51)
> alpha05 contains (0, 0, 0, 77)
> tmp contains (0, 51, 0, 128)
> step 4 results in (0, 204, 0, 127) which is an invalid pre-mul rgba pixel
> final result is (0, 255, 0, 255)
> 
> I think it's time to escalate to the SVG WG. The concrete proposal is to modify section 15.12 of the SVG 1.1 spec (16 August 2011) to change the statement:
> 
> "Additionally, a component-wise arithmetic operation (with the result clamped between [0..1]) can be applied."
> 
> to instead say
> 
> "Additionally, a component-wise arithmetic operation can be applied. Arithmetic results, including intermediate results, are clamped to alpha in [0,1] and color components in [0,alpha]."

How about you mail www-svg about this? I think we could get this resolved even for SVG 1.1 2nd edition, and if not, then for SVG2. I'm all for consistency, but if your approach is superior, we should convince the others. Not being an expert in filtering, there maybe some pros for the current approach, that I'm overlooking - hence we should ask the WG.

[Mike Reed]

Alternatively (or in addition), if we're going to open a discussion, I would like us to consider defining this mode to operate on nonpremultiplied components. This (imho) makes more sense, and has the nice side-effect of naturally handling the component-range issue "for free". If the src images are not already in nonpremul, it isn't free, but we already face that for some other effects (matrix, covolution), where it is more "correct" to operate in that space.

[Stephen White]

Comment on attachment 125011 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=125011&action=review

> Source/WebCore/platform/graphics/filters/FEComposite.cpp:136
> +        unsigned char a1 = *(source + 3);
> +        unsigned char a2 = *(destination + 3);

Perf note:  it's often much more efficient to load 32 bits at a time, and extract pixel color components with shifts than to do 4 8-bit loads (on x86 at least).

[Stephen Chenney]

(In reply to comment #27)
> (From update of attachment 125011 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=125011&action=review
> 
> > Source/WebCore/platform/graphics/filters/FEComposite.cpp:136
> > +        unsigned char a1 = *(source + 3);
> > +        unsigned char a2 = *(destination + 3);
> 
> Perf note:  it's often much more efficient to load 32 bits at a time, and extract pixel color components with shifts than to do 4 8-bit loads (on x86 at least).

Is that safe across different endian machines?

[Stephen Chenney]

Created attachment 125153 [details]
Patch

[WebKit Review Bot]

Attachment 125153 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

Last 3072 characters of output:
lock.html  [test/expectations] [2]
LayoutTests/platform/mac/test_expectations.txt:198:  Path does not exist. fast/dom/HTMLTableElement/colSpan.html  [test/expectations] [2]
LayoutTests/platform/mac/test_expectations.txt:199:  Path does not exist. fast/dom/HTMLTableElement/createCaption.html  [test/expectations] [2]
LayoutTests/platform/mac/test_expectations.txt:200:  Path does not exist. fast/repaint/table-section-repaint.html  [test/expectations] [2]
LayoutTests/platform/mac/test_expectations.txt:201:  Path does not exist. fast/table/frame-and-rules.html  [test/expectations] [2]
LayoutTests/platform/mac/test_expectations.txt:204:  Path does not exist. http/tests/media/video-buffering-repaints-controls.html  [test/expectations] [2]
LayoutTests/platform/mac/test_expectations.txt:207:  Path does not exist. fast/table/027.html  [test/expectations] [2]
LayoutTests/platform/mac/test_expectations.txt:208:  Path does not exist. fast/table/027-vertical.html  [test/expectations] [2]
Traceback (most recent call last):
  File "Tools/Scripts/check-webkit-style", line 48, in <module>
    sys.exit(CheckWebKitStyle().main())
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/main.py", line 154, in main
    patch_checker.check(patch)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/patchreader.py", line 66, in check
    self._text_file_reader.process_file(file_path=path, line_numbers=line_numbers)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/filereader.py", line 130, in process_file
    self._processor.process(lines, file_path, **kwargs)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checker.py", line 826, in process
    checker.check(lines)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checkers/test_expectations.py", line 119, in check
    overrides=overrides)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checkers/test_expectations.py", line 96, in check_test_expectations
    is_lint_mode=True, overrides=overrides)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py", line 726, in __init__
    self._add_skipped_tests(port.skipped_tests(tests))
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/layout_tests/port/webkit.py", line 376, in skipped_tests
    return self.skipped_layout_tests(test_list)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/layout_tests/port/webkit.py", line 372, in skipped_layout_tests
    tests_to_skip.update(self._skipped_tests_for_unsupported_features(test_list))
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/layout_tests/port/webkit.py", line 319, in _skipped_tests_for_unsupported_features
    if self._has_test_in_directories(self._missing_feature_to_skipped_tests().values(), test_list):
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/layout_tests/port/webkit.py", line 311, in _has_test_in_directories
    for directory, test in itertools.product(directories, test_list):
TypeError: 'NoneType' object is not iterable


If any of these errors are false positives, please file a bug against check-webkit-style.

[Stephen Chenney]

Here's another patch for your consideration. This one allows for arbitrary  intermediate results in a sequence of arithmetic filter operations while forcing the results passed to any other filter type are valid. Of course it touches all filters, hence the large patch.

So this allows us to match FF and Opera behavior on sequences of arithmetic filters, while not polluting the rest of the filter tree with garbage pixels.

I need to spend some quality time drafting emails to W3 SCG to see if we can get more rational specified behavior.

[Stephen White]

(In reply to comment #28)
> (In reply to comment #27)
> > (From update of attachment 125011 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=125011&action=review
> > 
> > > Source/WebCore/platform/graphics/filters/FEComposite.cpp:136
> > > +        unsigned char a1 = *(source + 3);
> > > +        unsigned char a2 = *(destination + 3);
> > 
> > Perf note:  it's often much more efficient to load 32 bits at a time, and extract pixel color components with shifts than to do 4 8-bit loads (on x86 at least).
> 
> Is that safe across different endian machines?

Good question.  Skia has endian-independent macros to extract the color components, with the order set at compile-time.  I can't find anything equivalent for WebKit, but perhaps I'm not looking hard enough.  It wouldn't be too hard to write them, if we want to be endian-correct.  There seems to be code in platform/graphics/cg that assumes little-endian, but suppose that's cool for MacOS now that it's all Intel.

I'm not even sure we have a machine that tests in big-endian mode.  Does anyone know if the WebKit bots run in big-endian mode?  One of the ARM bots maybe?

[Nikolas Zimmermann]

(In reply to comment #31)
> Here's another patch for your consideration. This one allows for arbitrary  intermediate results in a sequence of arithmetic filter operations while forcing the results passed to any other filter type are valid. Of course it touches all filters, hence the large patch.
> 
> So this allows us to match FF and Opera behavior on sequences of arithmetic filters, while not polluting the rest of the filter tree with garbage pixels.
This seems like a great compromise.

> 
> I need to spend some quality time drafting emails to W3 SCG to see if we can get more rational specified behavior.
Okay.

Do you want to continue investigating in loading 32bit in a row? Or shall it be left as-is? Did you measure perf diffs?

[Stephen Chenney]

(In reply to comment #33)
> Do you want to continue investigating in loading 32bit in a row? Or shall it be left as-is? Did you measure perf diffs?

I believe we should create a separate bug for the switch to 32bit color packing with a view to modifying all of the intermediate image handling inside SVG filters. Given the priority in moving to hardware acceleration, it's not clear to me that the work is worth the benefits.

This patch should stay as-is and remain consistent with other code.

I have not done formal perf testing. Can you point me to any information on how to go about that? While I can test it using the new perf testing builds (I think), there may be a better way.

[Stephen Chenney]

Created attachment 126139 [details]
Patch

This patch fixes the mac build, I hope. I'm really not concerned about performance here: we don't do anything significantly different except in the one case of arithmetic inputing to something else. Even then I think the performance will be close to before, maybe even better. Hence, I feel we can commit this.

[Nikolas Zimmermann]

Comment on attachment 126139 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=126139&action=review

Almost there, still some nitpicks:

> Source/WebCore/platform/graphics/filters/FEComposite.cpp:275
> +    if (validPreMulRGBAIsRequired)

Please use an enum here, the magic bool is not so nice.

Also I'd store validPreMulRGBAIsREquired in FilterEffect, to avoid touching all platformApplySoftware methods.

[Stephen Chenney]

Comment on attachment 126139 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=126139&action=review

>> Source/WebCore/platform/graphics/filters/FEComposite.cpp:275
>> +    if (validPreMulRGBAIsRequired)
> 
> Please use an enum here, the magic bool is not so nice.
> 
> Also I'd store validPreMulRGBAIsREquired in FilterEffect, to avoid touching all platformApplySoftware methods.

It's not obvious how to avoid the method argument. The requirement for a valid result is a property of the "parent" of the filter, or the consumer of its output. A given filter cannot decide on the requirement based on its own state. However, the only filter that does not require valid data is the arithmetic filter. Maybe I can come up with something that only requires changes in that class.

[Stephen Chenney]

Created attachment 129961 [details]
Patch

A much nicer solution. Thanks to Niko for pushing for something better.

[Stephen Chenney]

Created attachment 129963 [details]
Patch

Even nicer solution. Got rid uf an unnecessary local variable.

[Stephen White]

Comment on attachment 129963 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=129963&action=review

Looks good to me, but I'll leave for the SVG guys to take a look.

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:142
> +void FilterEffect::forceValidPreMulRGBA()
> +{
> +    if (!m_premultipliedImageResult)
> +        return;
> +
> +    ByteArray* imageArray = m_premultipliedImageResult.get();
> +    unsigned char* pixelData = imageArray->data();
> +    int pixelArrayLength = imageArray->length();
> +
> +    ASSERT(!(pixelArrayLength % 4));
> +    int numPixels = pixelArrayLength / 4;
> +    while (--numPixels >= 0) {
> +        unsigned char a = *(pixelData + 3);
> +        for (int i = 0; i < 3; ++i) {
> +            if (*pixelData > a)
> +                *pixelData = a;
> +            ++pixelData;
> +        }
> +        ++pixelData;
> +    }

Just to be clear, this function will only be called when going from an arithmetic FEComposite to something that is not an arithmetic FEComposite, correct?

[Stephen Chenney]

(In reply to comment #40)
> (From update of attachment 129963 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=129963&action=review
> 
> Looks good to me, but I'll leave for the SVG guys to take a look.
> 
> > Source/WebCore/platform/graphics/filters/FilterEffect.cpp:142
> > +void FilterEffect::forceValidPreMulRGBA()
> 
> Just to be clear, this function will only be called when going from an arithmetic FEComposite to something that is not an arithmetic FEComposite, correct?

Yes, and I verified by putting ASSERT(false) in which only hit during arithmetic filter layout tests.

[Nikolas Zimmermann]

Comment on attachment 129963 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=129963&action=review

Patch looks great! I still have some suggestions. First: The testcase is perfectly suited for a ref test, and I encourage you to rewrite it.

> Source/WebCore/platform/graphics/filters/FEComposite.h:62
> +    virtual bool requiresValidPreMulRGBA() OVERRIDE { return m_type != FECOMPOSITE_OPERATOR_ARITHMETIC; }

We shouldn't use abbreviations in WebKit per coding style guidelines, how about requiresValidPremultipliedRGBA? or PreMultiplied if you prefer that.
(I'd grep for similar names in WebCore, and see what's used)

> Source/WebCore/platform/graphics/filters/FEComposite.h:63
> +    virtual bool isInvalidPreMulRGBA() OVERRIDE { return m_type == FECOMPOSITE_OPERATOR_ARITHMETIC; }

This method is not really needed, you always use it like this:
for (unsigned i = 0; i < size; ++i) {
    FilterEffect* in = m_inputEffects.at(i).get();
    if (in->isInvalidPreMulRGBA())
        in->forceValidPreMulRGBA();
}
How about switching to:

for (unsigned i = 0; i < size; ++i)
    m_inputEffects[i]->forceValidPreMultipliedRGBAOutputIfNeeded();

Or even "sanitzeEffectIfNeeded" to hide the detail, what that involves.

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:96
>          return;

You could leave that out.

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:127
> +

This function should be commented!

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:136
> +    int numPixels = pixelArrayLength / 4;
> +    while (--numPixels >= 0) {
> +        unsigned char a = *(pixelData + 3);
> +        for (int i = 0; i < 3; ++i) {

While I verified this is correct, comments would be helpful :-)

> LayoutTests/platform/chromium/test_expectations.txt:943
> +BUGWK77245 : svg/filters/feComposite-arithmetic-invalid-rgba.svg = PASS FAIL

This could be fully avoided with a reftest.

> LayoutTests/svg/filters/feComposite-arithmetic-invalid-rgba.svg:1
> +<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="200" height="100" style="background:white;">

Heh, background: white works? Never tried that :-) Is it needed?

[Stephen Chenney]

Comment on attachment 129963 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=129963&action=review

New patch shortly.

>> Source/WebCore/platform/graphics/filters/FEComposite.h:62
>> +    virtual bool requiresValidPreMulRGBA() OVERRIDE { return m_type != FECOMPOSITE_OPERATOR_ARITHMETIC; }
> 
> We shouldn't use abbreviations in WebKit per coding style guidelines, how about requiresValidPremultipliedRGBA? or PreMultiplied if you prefer that.
> (I'd grep for similar names in WebCore, and see what's used)

Sounds good. All this info is very helpful in helping me reduce mistakes.

>> Source/WebCore/platform/graphics/filters/FEComposite.h:63
>> +    virtual bool isInvalidPreMulRGBA() OVERRIDE { return m_type == FECOMPOSITE_OPERATOR_ARITHMETIC; }
> 
> This method is not really needed, you always use it like this:
> for (unsigned i = 0; i < size; ++i) {
>     FilterEffect* in = m_inputEffects.at(i).get();
>     if (in->isInvalidPreMulRGBA())
>         in->forceValidPreMulRGBA();
> }
> How about switching to:
> 
> for (unsigned i = 0; i < size; ++i)
>     m_inputEffects[i]->forceValidPreMultipliedRGBAOutputIfNeeded();
> 
> Or even "sanitzeEffectIfNeeded" to hide the detail, what that involves.

Good point. Will do.

>> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:96
>>          return;
> 
> You could leave that out.

I did not want to change existing code. I'll look at it to see if this can ever be called with an existing, valid result.

>> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:127
>> +
> 
> This function should be commented!

Here you mean? Sure.

>> LayoutTests/platform/chromium/test_expectations.txt:943
>> +BUGWK77245 : svg/filters/feComposite-arithmetic-invalid-rgba.svg = PASS FAIL
> 
> This could be fully avoided with a reftest.

Now why didn't I think of that?

>> LayoutTests/svg/filters/feComposite-arithmetic-invalid-rgba.svg:1
>> +<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="200" height="100" style="background:white;">
> 
> Heh, background: white works? Never tried that :-) Is it needed?

Not needed. At one point I had a colored background and forgot to fully remove it.

[Stephen Chenney]

Created attachment 130146 [details]
Patch

New patch reflecting Niko's comments. Testing is now simpler.

[WebKit Review Bot]

Comment on attachment 130146 [details]
Patch

Attachment 130146 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11819075

New failing tests:
svg/filters/feComposite-arithmetic-invalid-rgba.svg

[Stephen Chenney]

Comment on attachment 130146 [details]
Patch

Apparently Chrome Linux is rounding the color values differently.

[Nikolas Zimmermann]

Comment on attachment 130146 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130146&action=review

In general this patch looks much better, I'd r+ it once you fix the cr-linux problem. Good job!

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:97
> +

I meant to leave out this whitespace only change before :-)

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:122
> +    // Must operate on pre-multiplied results - other formats cannot have invalid pixels

Sentences should end with a trailing . in general.

[Stephen Chenney]

Created attachment 130196 [details]
Patch

Fixed the ref test. Note https://bugs.webkit.org/show_bug.cgi?id=80321

[Stephen White]

Comment on attachment 130196 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130196&action=review

Looks good.  r=me

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:119
> +void FilterEffect::forceValidPreMultipliedPixels()

Nit:  It looks like the guts of this function could potentially live in FEComposite::correctFilterResultIfNeeded, since it's the only caller.  Not a big deal, though.

[WebKit Review Bot]

Comment on attachment 130196 [details]
Patch

Clearing flags on attachment: 130196

Committed r109820: <http://trac.webkit.org/changeset/109820>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.