Bug 97859

I'll submit a patch today.

Created attachment 166171 [details] Patch

Created attachment 166180 [details] Patch

(In reply to comment #3) > Created an attachment (id=166180) [details] > Patch css3/filters/custom/custom-filter-composite-operators.html failed because rgba(75%, 37.5%, 50%, 0.8) can not be represented by 8-bit accurately. So I baselined the result color values using the same way in Bug 93870.

Comment on attachment 166180 [details] Patch Attachment 166180 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14068157 New failing tests: css3/filters/custom/custom-filter-composite-operators.html css3/filters/custom/custom-filter-property-parsing-invalid.html

Comment on attachment 166180 [details] Patch Attachment 166180 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14068160 New failing tests: css3/filters/custom/custom-filter-composite-operators.html css3/filters/custom/custom-filter-property-parsing-invalid.html

Created attachment 166202 [details] Patch

Comment on attachment 166202 [details] Patch Attachment 166202 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14068210 New failing tests: css3/filters/custom/custom-filter-composite-operators.html

Comment on attachment 166202 [details] Patch Attachment 166202 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14059386 New failing tests: css3/filters/custom/custom-filter-composite-operators.html

Comment on attachment 166202 [details] Patch Thanks for the patch! Looking good. I have a couple of comments: View in context: https://bugs.webkit.org/attachment.cgi?id=166202&action=review > Source/WebCore/ChangeLog:3 > + [CSS Shaders] Implement all composite operators. You haven't implemented "destination" from the Compositing and Blending spec [1], so this title should probably say "Implement all composite operators except destination". We'll need to figure out what to do with destination eventually, but it doesn't have to be in this bug :). We'll need a new CompositeOperator, CompositeDestination. Unrelated to the shaders implementation, there's no kCGBlendModeDestination in CoreGraphics [2]. [1]: https://dvcs.w3.org/hg/FXTF/rawfile/tip/compositing/index.html#alpha-compositing [2]: Search for "CGBlendMode" in http://developer.apple.com/library/ios/#documentation/GraphicsImaging/Reference/CGContext/Reference/reference.html > Source/WebCore/css/CSSParser.cpp:-7467 > - // FIXME: Add CSSValueDestination and CSSValueLighter when the Compositing spec updates. You haven't implemented "destination", so this FIXME needs to be updated to say that. It shouldn't be removed quite yet. > Source/WebCore/css/CSSParser.cpp:7467 > + return ident >= CSSValueClear && ident <= CSSValuePlusLighter && ident != CSSValuePlusDarker; @achicu, @dino: Can one of you comment on the following issue? According to the Compositing and Blending spec [1], "plus-lighter" is not a valid CSS value. You want "lighter". However, we can't just add the "lighter" CSS value and map it to CompositePlusLighter. The CSS value "plus-lighter" for -webkit-background-composite is already mapped to CompositePlusLighter in CSSPrimitiveValueMappings.h [3]. We have a couple of options for resolving this: (1) Change the CSS value "plus-lighter" to "lighter" and break -webkit-background-composite. Then, we should probably also change "plus-darker" to "darker". (2) Keep the CSS value "plus-lighter". Add the CSS value "lighter". Add CompositeOperatorLighter. Then, we have two synonymous CompositeOperators, CompositeOperatorPlusLighter and CompositeOperatorLighter. Platform code might have to deal with these two CompositeOperators, which is not ideal. (3) Create separate CSSValue <-> CompositeOperator mappings for -webkit-background-composite and shaders/CSS compositing. For shaders, we would have CSSValueLighter <-> CompositeOperatorPlusLighter. For -webkit-background-composite, we would have CSSValuePlusLighter <-> CompositeOperatorPlusLighter. I'm not sure how to implement this nicely, though. [3]: Search for "template<> inline CSSPrimitiveValue::operator CompositeOperator() const" in CSSPrimitiveValueMappings.h. > Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:251 > case CompositeClear: The expressions look good. > LayoutTests/css3/filters/custom/custom-filter-composite-operators.html:5 > + <!-- This test passes if each horizontal pair of squares are exactly the same color. --> @achicu, @dino: Could one of you chime in on this following point? I wouldn't want to make Huang rewrite his tests if it's just my opinion. This type of test is particularly good for testing blend modes, which define how to mix two RGB colors. However, I'm not sure it's the best way to test alpha compositing operators. Conventionally, alpha compositing operators are related to which parts of the resulting image come from the source (css_MixColor) and which parts come from the destination (the element texture), based on the alpha channel. That's why the spec [4] shows examples that composite an offset blue square surrounded by transparency with a differently offset yellow square surrounded by transparency. If you look at our existing compositing test for "source-atop" [5], you'll see we use that technique. I would prefer that we use that same technique to test all of the compositing operators. It makes it easy to visually verify the correctness of the test, since it looks exactly like the classic compositing images from the spec. Also, each test case verifies multiple different combinations of alpha values for each composite operator, instead of just one combination per composite operator. I do like that you used a ref test instead of a pixel test, and I like that you put all of the composite operators in this single ref test. I suggest you keep the same overall structure, where each row is a test case, and the left side and right side of each row must match. However, instead of single-color squares, each "square" should look like one of compositing result images from the spec. In the test file, the left image can be rendered with shaders, and the right image can be rendered with divs. In the reference file, both sides can be rendered with divs. [4]: https://dvcs.w3.org/hg/FXTF/rawfile/tip/compositing/index.html#porterduffcompositingoperators_clear [5]: LayoutTests/css3/filters/custom/custom-filter-composite-source-atop.html > LayoutTests/css3/filters/custom/custom-filter-composite-operators.html:47 > + Fa = αb; Fb = 1 â αs I'm seeing non-ASCII characters in these expressions on my machine, such as a "plus equals" sign [6]. [6]: http://www.w3.org/TR/MathML2/glyphs/02A/U02A72.png > LayoutTests/css3/filters/custom/custom-filter-composite-operators.html:279 > + -webkit-filter: custom(none mix(url('../resources/mix-color.fs') normal plus-lighter), mix_color 0.0 0.6 0.5 0.5); Same as before. "plus-lighter" is not a valid CSS compositing operator according to the spec. > LayoutTests/css3/filters/custom/custom-filter-property-parsing-invalid-expected.txt:-82 > -Mix function with alpha compositing mode 'plus-lighter', which should only apply to -webkit-background-composite : custom(none mix(url(shader) plus-lighter)) Please keep this text expectation in place. > LayoutTests/css3/filters/script-tests/custom-filter-property-parsing-invalid.js:-46 > -testInvalidFilterRule("Mix function with alpha compositing mode 'plus-lighter', which should only apply to -webkit-background-composite", "custom(none mix(url(shader) plus-lighter))"); Please keep this test case in place.

(In reply to comment #10) > (From update of attachment 166202 [details]) > Thanks for the patch! Looking good. I have a couple of comments: Thanks for your review! > You haven't implemented "destination", so this FIXME needs to be updated to say that. It shouldn't be removed quite yet. > > > Source/WebCore/css/CSSParser.cpp:7467 > > + return ident >= CSSValueClear && ident <= CSSValuePlusLighter && ident != CSSValuePlusDarker; I handled plus-lighter because CompositePlusLighter enum was in CustomFilterValidatedProgram.cpp As reading your comments, plus-lighter should be postponed like destination. Do you think it is better that I file another bug about plus-lighter and destination? > > According to the Compositing and Blending spec [1], "plus-lighter" is not a valid CSS value. You want "lighter". I did not recognize the difference between "plus-lighter" and "lighter" due to the lack of knowledge. I see now. > Conventionally, alpha compositing operators are related to which parts of the resulting image come from the source (css_MixColor) and which parts come from the destination (the element texture), based on the alpha channel. > > That's why the spec [4] shows examples that composite an offset blue square surrounded by transparency with a differently offset yellow square surrounded by transparency. If you look at our existing compositing test for "source-atop" [5], you'll see we use that technique. I agree with your opinion. I'll try to make new test like "source-atop" test. However, I can submit new patch after about 5 days because Korea is on Thanksgiving from today. By the way, cr-linux and mac bots failed this test after I adjusted the pixels from GTK results. cr-linux and mac bots had failed also the test in the second patch that I adjusted the pixels from Qt results. I think this kind of tests will be flaky when other ports unskip them. I'm curious whether other tests in css3/filters/custom have similar problems. > > LayoutTests/css3/filters/script-tests/custom-filter-property-parsing-invalid.js:-46 > > -testInvalidFilterRule("Mix function with alpha compositing mode 'plus-lighter', which should only apply to -webkit-background-composite", "custom(none mix(url(shader) plus-lighter))"); > > Please keep this test case in place. I'll rollback CSSParcer to not handle 'plus-lighter' and rollback this test also.

(In reply to comment #11) > (In reply to comment #10) > > (From update of attachment 166202 [details] [details]) > > Thanks for the patch! Looking good. I have a couple of comments: > > Thanks for your review! > > > You haven't implemented "destination", so this FIXME needs to be updated to say that. It shouldn't be removed quite yet. > > > > > Source/WebCore/css/CSSParser.cpp:7467 > > > + return ident >= CSSValueClear && ident <= CSSValuePlusLighter && ident != CSSValuePlusDarker; > > I handled plus-lighter because CompositePlusLighter enum was in CustomFilterValidatedProgram.cpp > As reading your comments, plus-lighter should be postponed like destination. Do you think it is better that I file another bug about plus-lighter and destination? Yes, that sounds like a good idea. Destination and lighter are tricky enough to be their own bugs. > > > > > According to the Compositing and Blending spec [1], "plus-lighter" is not a valid CSS value. You want "lighter". > > I did not recognize the difference between "plus-lighter" and "lighter" due to the lack of knowledge. I see now. No problem. These are really subtle :) > > > Conventionally, alpha compositing operators are related to which parts of the resulting image come from the source (css_MixColor) and which parts come from the destination (the element texture), based on the alpha channel. > > > > That's why the spec [4] shows examples that composite an offset blue square surrounded by transparency with a differently offset yellow square surrounded by transparency. If you look at our existing compositing test for "source-atop" [5], you'll see we use that technique. > > I agree with your opinion. I'll try to make new test like "source-atop" test. However, I can submit new patch after about 5 days because Korea is on Thanksgiving from today. Ok, great. Thanks for doing that- I know it's a lot more work. You can take your time, since we don't have any dependent bugs on this one. Enjoy your vacation :) > > By the way, cr-linux and mac bots failed this test after I adjusted the pixels from GTK results. cr-linux and mac bots had failed also the test in the second patch that I adjusted the pixels from Qt results. I think this kind of tests will be flaky when other ports unskip them. I'm curious whether other tests in css3/filters/custom have similar problems. The other tests might have problems. Using rgb colors with 8-bit values instead of floats helped me in custom-filter-blend-modes.html. Hopefully, after you change the test, you won't have as many problems because you just have to compare two simple colors (blue and yellow). > > > > LayoutTests/css3/filters/script-tests/custom-filter-property-parsing-invalid.js:-46 > > > -testInvalidFilterRule("Mix function with alpha compositing mode 'plus-lighter', which should only apply to -webkit-background-composite", "custom(none mix(url(shader) plus-lighter))"); > > > > Please keep this test case in place. > > I'll rollback CSSParcer to not handle 'plus-lighter' and rollback this test also. Sounds good. Thanks!

Created attachment 167061 [details] Patch

(In reply to comment #12) > Yes, that sounds like a good idea. Destination and lighter are tricky enough to be their own bugs. I filed Bug 98391. > Ok, great. Thanks for doing that- I know it's a lot more work. You can take your time, since we don't have any dependent bugs on this one. Enjoy your vacation :) Thank you for your kindness :) I made new test similar to css3/filters/custom/custom-filter-composite-source-atop.html as you suggested. > > I'll rollback CSSParcer to not handle 'plus-lighter' and rollback this test also. > > Sounds good. Thanks! When destination and lighter are implemented, CSSParcer will be changed. Thanks.

Created attachment 167067 [details] Patch

(In reply to comment #15) > Created an attachment (id=167067) [details] > Patch Fixed typo in comments and Changelog.

Comment on attachment 167067 [details] Patch Looking great! Thanks for the updates. View in context: https://bugs.webkit.org/attachment.cgi?id=167067&action=review > Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:310 > + return vec4((as * Fa * Cs + ab * Fb * Cb) / ao, ao); This change is unrelated to the compositing modes, so I would take it out. I think you should land bug 98396 first, since you're removing this change in there. That is, refactor first, and then add new functionality. > LayoutTests/css3/filters/custom/custom-filter-composite-operators.html:4 > + <title>Tests that custom filter composite operators compute color values correctly.</title> This test looks great! Thanks for making it. Do you think it can be a ref test instead of a pixel test?

Created attachment 167183 [details] Patch

(In reply to comment #17) > This change is unrelated to the compositing modes, so I would take it out. I think you should land bug 98396 first, since you're removing this change in there. That is, refactor first, and then add new functionality. Absolutely! > This test looks great! Thanks for making it. Do you think it can be a ref test instead of a pixel test? I made it because of your help. Unfortunately, it seems not to be an alternative method of a pixel test IMHO. In my experience of layout tests, only canvas element has an alternative method because the canvas has getImageData API. I wish listening other forks' opinions too.

Comment on attachment 167183 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167183&action=review > Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:294 > case CompositePlusLighter: > notImplemented(); > return String(); > + // FIXME: destination and lighter were not implemented yet. I don't think we need this comment. It's clear that they are not implemented. > LayoutTests/css3/filters/custom/custom-filter-composite-operators.html:4 > + <title>Tests that custom filter composite operators compute color values correctly.</title> As Max originally suggested, it would be great if this could be a reftest, with the expected value shown next to each subtest. I wonder if we can be accurate enough by placing colored divs to replicate the blending. Anyway, this is ok for now.

Created attachment 167957 [details] Patch

(In reply to comment #20) > I don't think we need this comment. It's clear that they are not implemented. Done. > As Max originally suggested, it would be great if this could be a reftest, with the expected value shown next to each subtest. I wonder if we can be accurate enough by placing colored divs to replicate the blending. Anyway, this is ok for now. I agree. Thanks for your review. Could you check r+ c+ one more time? You already checked r+. I'm not commiter, so I always depend on commit-queue-bot.

Comment on attachment 167957 [details] Patch Clearing flags on attachment: 167957 Committed r131100: <http://trac.webkit.org/changeset/131100>

All reviewed patches have been landed. Closing bug.

This patch caused an ASSERT: crash log for DumpRenderTree (pid 1352): STDOUT: <empty> STDERR: ASSERTION FAILED: m_samplerLocation != -1 STDERR: /Volumes/data/b/build/slave/webkit-mac-latest-dbg/build/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../platform/graphics/filters/CustomFilterCompiledProgram.cpp(139) : void WebCore::CustomFilterCompiledProgram::initializeParameterLocations(WebCore::CustomFilterProgramType) STDERR: 1 0x24a96b14 WebCore::CustomFilterCompiledProgram::initializeParameterLocations(WebCore::CustomFilterProgramType) STDERR: 2 0x24a9624b WebCore::CustomFilterCompiledProgram::CustomFilterCompiledProgram(WTF::PassRefPtr<WebCore::GraphicsContext3D>, WTF::String const&, WTF::String const&, WebCore::CustomFilterProgramType) STDERR: 3 0x24a9603c WebCore::CustomFilterCompiledProgram::CustomFilterCompiledProgram(WTF::PassRefPtr<WebCore::GraphicsContext3D>, WTF::String const&, WTF::String const&, WebCore::CustomFilterProgramType) STDERR: 4 0x24a984cb WebCore::CustomFilterCompiledProgram::create(WTF::PassRefPtr<WebCore::GraphicsContext3D>, WTF::String const&, WTF::String const&, WebCore::CustomFilterProgramType) STDERR: 5 0x24a97944 WebCore::CustomFilterValidatedProgram::compiledProgram() STDERR: 6 0x24aaad86 WebCore::FECustomFilter::initializeContext() STDERR: 7 0x24aaab65 WebCore::FECustomFilter::prepareForDrawing(WebCore::FECustomFilter::CustomFilterDrawType) STDERR: 8 0x24aa941f WebCore::FECustomFilter::applyShader() STDERR: 9 0x24aa932e WebCore::FECustomFilter::platformApplySoftware() STDERR: 10 0x24ac4396 WebCore::FilterEffect::apply() STDERR: 11 0x25a367ab WebCore::FilterEffectRenderer::apply() STDERR: 12 0x25a36f64 WebCore::FilterEffectRendererHelper::applyFilterEffect() STDERR: 13 0x25bea756 WebCore::RenderLayer::paintLayerContents(WebCore::RenderLayer*, WebCore::GraphicsContext*, WebCore::FractionalLayoutRect const&, WebCore::FractionalLayoutSize const&, unsigned int, WebCore::RenderObject*, WebCore::RenderRegion*, WTF::HashMap<WebCore::OverlapTestRequestClient*, WebCore::IntRect, WTF::PtrHash<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::IntRect> >*, unsigned int) STDERR: 14 0x25be921b WebCore::RenderLayer::paintLayerContentsAndReflection(WebCore::RenderLayer*, WebCore::GraphicsContext*, WebCore::FractionalLayoutRect const&, WebCore::FractionalLayoutSize const&, unsigned int, WebCore::RenderObject*, WebCore::RenderRegion*, WTF::HashMap<WebCore::OverlapTestRequestClient*, WebCore::IntRect, WTF::PtrHash<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::IntRect> >*, unsigned int) STDERR: 15 0x25be852c WebCore::RenderLayer::paintLayer(WebCore::RenderLayer*, WebCore::GraphicsContext*, WebCore::FractionalLayoutRect const&, WebCore::FractionalLayoutSize const&, unsigned int, WebCore::RenderObject*, WebCore::RenderRegion*, WTF::HashMap<WebCore::OverlapTestRequestClient*, WebCore::IntRect, WTF::PtrHash<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::IntRect> >*, unsigned int) STDERR: 16 0x25beb635 WebCore::RenderLayer::paintList(WTF::Vector<WebCore::RenderLayer*, 0ul>*, WebCore::RenderLayer*, WebCore::GraphicsContext*, WebCore::FractionalLayoutRect const&, unsigned int, WebCore::RenderObject*, WebCore::RenderRegion*, WTF::HashMap<WebCore::OverlapTestRequestClient*, WebCore::IntRect, WTF::PtrHash<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::IntRect> >*, unsigned int) STDERR: 17 0x25bea5fa WebCore::RenderLayer::paintLayerContents(WebCore::RenderLayer*, WebCore::GraphicsContext*, WebCore::FractionalLayoutRect const&, WebCore::FractionalLayoutSize const&, unsigned int, WebCore::RenderObject*, WebCore::RenderRegion*, WTF::HashMap<WebCore::OverlapTestRequestClient*, WebCore::IntRect, WTF::PtrHash<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::IntRect> >*, unsigned int) STDERR: 18 0x25be921b WebCore::RenderLayer::paintLayerContentsAndReflection(WebCore::RenderLayer*, WebCore::GraphicsContext*, WebCore::FractionalLayoutRect const&, WebCore::FractionalLayoutSize const&, unsigned int, WebCore::RenderObject*, WebCore::RenderRegion*, WTF::HashMap<WebCore::OverlapTestRequestClient*, WebCore::IntRect, WTF::PtrHash<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::IntRect> >*, unsigned int) STDERR: 19 0x25be852c WebCore::RenderLayer::paintLayer(WebCore::RenderLayer*, WebCore::GraphicsContext*, WebCore::FractionalLayoutRect const&, WebCore::FractionalLayoutSize const&, unsigned int, WebCore::RenderObject*, WebCore::RenderRegion*, WTF::HashMap<WebCore::OverlapTestRequestClient*, WebCore::IntRect, WTF::PtrHash<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::IntRect> >*, unsigned int) STDERR: 20 0x25beb635 WebCore::RenderLayer::paintList(WTF::Vector<WebCore::RenderLayer*, 0ul>*, WebCore::RenderLayer*, WebCore::GraphicsContext*, WebCore::FractionalLayoutRect const&, unsigned int, WebCore::RenderObject*, WebCore::RenderRegion*, WTF::HashMap<WebCore::OverlapTestRequestClient*, WebCore::IntRect, WTF::PtrHash<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::IntRect> >*, unsigned int) STDERR: 21 0x25bea5fa WebCore::RenderLayer::paintLayerContents(WebCore::RenderLayer*, WebCore::GraphicsContext*, WebCore::FractionalLayoutRect const&, WebCore::FractionalLayoutSize const&, unsigned int, WebCore::RenderObject*, WebCore::RenderRegion*, WTF::HashMap<WebCore::OverlapTestRequestClient*, WebCore::IntRect, WTF::PtrHash<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::IntRect> >*, unsigned int) STDERR: 22 0x25be921b WebCore::RenderLayer::paintLayerContentsAndReflection(WebCore::RenderLayer*, WebCore::GraphicsContext*, WebCore::FractionalLayoutRect const&, WebCore::FractionalLayoutSize const&, unsigned int, WebCore::RenderObject*, WebCore::RenderRegion*, WTF::HashMap<WebCore::OverlapTestRequestClient*, WebCore::IntRect, WTF::PtrHash<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::IntRect> >*, unsigned int) STDERR: 23 0x25be852c WebCore::RenderLayer::paintLayer(WebCore::RenderLayer*, WebCore::GraphicsContext*, WebCore::FractionalLayoutRect const&, WebCore::FractionalLayoutSize const&, unsigned int, WebCore::RenderObject*, WebCore::RenderRegion*, WTF::HashMap<WebCore::OverlapTestRequestClient*, WebCore::IntRect, WTF::PtrHash<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::IntRect> >*, unsigned int) STDERR: 24 0x25be7c5b WebCore::RenderLayer::paint(WebCore::GraphicsContext*, WebCore::FractionalLayoutRect const&, unsigned int, WebCore::RenderObject*, WebCore::RenderRegion*, unsigned int) STDERR: 25 0x2588a73c WebCore::FrameView::paintContents(WebCore::GraphicsContext*, WebCore::IntRect const&) STDERR: 26 0x2494fabc WebCore::ScrollView::paint(WebCore::GraphicsContext*, WebCore::IntRect const&) STDERR: 27 0x23516b7e WebKit::PageWidgetDelegate::paint(WebCore::Page*, WebKit::PageOverlayList*, SkCanvas*, WebKit::WebRect const&, WebKit::PageWidgetDelegate::CanvasBackground) STDERR: 28 0x23655e28 WebKit::WebViewImpl::paint(SkCanvas*, WebKit::WebRect const&, WebKit::WebWidget::PaintOptions) STDERR: 29 0x22baa308 WebViewHost::paintRect(WebKit::WebRect const&) STDERR: 30 0x22baa7c9 WebViewHost::paintInvalidatedRegion() STDERR: 31 0x22b88493 TestShell::dump() STDERR: [1352:-1602484928:365815664370:ERROR:process_util_posix.cc(144)] Received signal 11 STDERR: 0 libbase.dylib 0x2c05ef7f base::debug::StackTrace::StackTrace() + 63 STDERR: 1 libbase.dylib 0x2c05ef1b base::debug::StackTrace::StackTrace() + 43 STDERR: 2 libbase.dylib 0x2c13a407 base::(anonymous namespace)::StackDumpSignalHandler(int, __siginfo*, __darwin_ucontext*) + 295 STDERR: 3 libSystem.B.dylib 0x9539405b _sigtramp + 43 STDERR: 4 ??? 0xffffffff 0x0 + 4294967295 STDERR: 5 libwebkit.dylib 0x24a9624b WebCore::CustomFilterCompiledProgram::CustomFilterCompiledProgram(WTF::PassRefPtr<WebCore::GraphicsContext3D>, WTF::String const&, WTF::String const&, WebCore::CustomFilterProgramType) + 491 STDERR: 6 libwebkit.dylib 0x24a9603c WebCore::CustomFilterCompiledProgram::CustomFilterCompiledProgram(WTF::PassRefPtr<WebCore::GraphicsContext3D>, WTF::String const&, WTF::String const&, WebCore::CustomFilterProgramType) + 92 STDERR: 7 libwebkit.dylib 0x24a984cb WebCore::CustomFilterCompiledProgram::create(WTF::PassRefPtr<WebCore::GraphicsContext3D>, WTF::String const&, WTF::String const&, WebCore::CustomFilterProgramType) + 139 STDERR: 8 libwebkit.dylib 0x24a97944 WebCore::CustomFilterValidatedProgram::compiledProgram() + 356 STDERR: 9 libwebkit.dylib 0x24aaad86 WebCore::FECustomFilter::initializeContext() + 310 STDERR: 10 libwebkit.dylib 0x24aaab65 WebCore::FECustomFilter::prepareForDrawing(WebCore::FECustomFilter::CustomFilterDrawType) + 85 STDERR: 11 libwebkit.dylib 0x24aa941f WebCore::FECustomFilter::applyShader() + 175 STDERR: 12 libwebkit.dylib 0x24aa932e WebCore::FECustomFilter::platformApplySoftware() + 46 STDERR: 13 libwebkit.dylib 0x24ac4396 WebCore::FilterEffect::apply() + 438 STDERR: 14 libwebkit.dylib 0x25a367ab WebCore::FilterEffectRenderer::apply() + 75 STDERR: 15 libwebkit.dylib 0x25a36f64 WebCore::FilterEffectRendererHelper::applyFilterEffect() + 212 STDERR: 16 libwebkit.dylib 0x25bea756 WebCore::RenderLayer::paintLayerContents(WebCore::RenderLayer*, WebCore::GraphicsContext*, WebCore::FractionalLayoutRect const&, WebCore::FractionalLayoutSize const&, unsigned int, WebCore::RenderObject*, WebCore::RenderRegion*, WTF::HashMap<WebCore::OverlapTestRequestClient*, WebCore::IntRect, WTF::PtrHash<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::IntRect> >*, unsigned int) + 5382 STDERR: 17 libwebkit.dylib 0x25be921b WebCore::RenderLayer::paintLayerContentsAndReflection(WebCore::RenderLayer*, WebCore::GraphicsContext*, WebCore::FractionalLayoutRect const&, WebCore::FractionalLayoutSize const&, unsigned int, WebCore::RenderObject*, WebCore::RenderRegion*, WTF::HashMap<WebCore::OverlapTestRequestClient*, WebCore::IntRect, WTF::PtrHash<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::IntRect> >*, unsigned int) + 555 STDERR: 18 libwebkit.dylib 0x25be852c WebCore::RenderLayer::paintLayer(WebCore::RenderLayer*, WebCore::GraphicsContext*, WebCore::FractionalLayoutRect const&, WebCore::FractionalLayoutSize const&, unsigned int, WebCore::RenderObject*, WebCore::RenderRegion*, WTF::HashMap<WebCore::OverlapTestRequestClient*, WebCore::IntRect, WTF::PtrHash<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::IntRect> >*, unsigned int) + 2012 STDERR: 19 libwebkit.dylib 0x25beb635 WebCore::RenderLayer::paintList(WTF::Vector<WebCore::RenderLayer*, 0ul>*, WebCore::RenderLayer*, WebCore::GraphicsContext*, WebCore::FractionalLayoutRect const&, unsigned int, WebCore::RenderObject*, WebCore::RenderRegion*, WTF::HashMap<WebCore::OverlapTestRequestClient*, WebCore::IntRect, WTF::PtrHash<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::IntRect> >*, unsigned int) + 389 STDERR: 20 libwebkit.dylib 0x25bea5fa WebCore::RenderLayer::paintLayerContents(WebCore::RenderLayer*, WebCore::GraphicsContext*, WebCore::FractionalLayoutRect const&, WebCore::FractionalLayoutSize const&, unsigned int, WebCore::RenderObject*, WebCore::RenderRegion*, WTF::HashMap<WebCore::OverlapTestRequestClient*, WebCore::IntRect, WTF::PtrHash<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::IntRect> >*, unsigned int) + 5034 STDERR: 21 libwebkit.dylib 0x25be921b WebCore::RenderLayer::paintLayerContentsAndReflection(WebCore::RenderLayer*, WebCore::GraphicsContext*, WebCore::FractionalLayoutRect const&, WebCore::FractionalLayoutSize const&, unsigned int, WebCore::RenderObject*, WebCore::RenderRegion*, WTF::HashMap<WebCore::OverlapTestRequestClient*, WebCore::IntRect, WTF::PtrHash<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::IntRect> >*, unsigned int) + 555 STDERR: 22 libwebkit.dylib 0x25be852c WebCore::RenderLayer::paintLayer(WebCore::RenderLayer*, WebCore::GraphicsContext*, WebCore::FractionalLayoutRect const&, WebCore::FractionalLayoutSize const&, unsigned int, WebCore::RenderObject*, WebCore::RenderRegion*, WTF::HashMap<WebCore::OverlapTestRequestClient*, WebCore::IntRect, WTF::PtrHash<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::IntRect> >*, unsigned int) + 2012 STDERR: 23 libwebkit.dylib 0x25beb635 WebCore::RenderLayer::paintList(WTF::Vector<WebCore::RenderLayer*, 0ul>*, WebCore::RenderLayer*, WebCore::GraphicsContext*, WebCore::FractionalLayoutRect const&, unsigned int, WebCore::RenderObject*, WebCore::RenderRegion*, WTF::HashMap<WebCore::OverlapTestRequestClient*, WebCore::IntRect, WTF::PtrHash<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::IntRect> >*, unsigned int) + 389 STDERR: 24 libwebkit.dylib 0x25bea5fa WebCore::RenderLayer::paintLayerContents(WebCore::RenderLayer*, WebCore::GraphicsContext*, WebCore::FractionalLayoutRect const&, WebCore::FractionalLayoutSize const&, unsigned int, WebCore::RenderObject*, WebCore::RenderRegion*, WTF::HashMap<WebCore::OverlapTestRequestClient*, WebCore::IntRect, WTF::PtrHash<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::IntRect> >*, unsigned int) + 5034 STDERR: 25 libwebkit.dylib 0x25be921b WebCore::RenderLayer::paintLayerContentsAndReflection(WebCore::RenderLayer*, WebCore::GraphicsContext*, WebCore::FractionalLayoutRect const&, WebCore::FractionalLayoutSize const&, unsigned int, WebCore::RenderObject*, WebCore::RenderRegion*, WTF::HashMap<WebCore::OverlapTestRequestClient*, WebCore::IntRect, WTF::PtrHash<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::IntRect> >*, unsigned int) + 555 STDERR: 26 libwebkit.dylib 0x25be852c WebCore::RenderLayer::paintLayer(WebCore::RenderLayer*, WebCore::GraphicsContext*, WebCore::FractionalLayoutRect const&, WebCore::FractionalLayoutSize const&, unsigned int, WebCore::RenderObject*, WebCore::RenderRegion*, WTF::HashMap<WebCore::OverlapTestRequestClient*, WebCore::IntRect, WTF::PtrHash<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::IntRect> >*, unsigned int) + 2012 STDERR: 27 libwebkit.dylib 0x25be7c5b WebCore::RenderLayer::paint(WebCore::GraphicsContext*, WebCore::FractionalLayoutRect const&, unsigned int, WebCore::RenderObject*, WebCore::RenderRegion*, unsigned int) + 299 STDERR: 28 libwebkit.dylib 0x2588a73c WebCore::FrameView::paintContents(WebCore::GraphicsContext*, WebCore::IntRect const&) + 1708 STDERR: 29 libwebkit.dylib 0x2494fabc WebCore::ScrollView::paint(WebCore::GraphicsContext*, WebCore::IntRect const&) + 1068 STDERR: 30 libwebkit.dylib 0x23516b7e WebKit::PageWidgetDelegate::paint(WebCore::Page*, WebKit::PageOverlayList*, SkCanvas*, WebKit::WebRect const&, WebKit::PageWidgetDelegate::CanvasBackground) + 462 STDERR: 31 libwebkit.dylib 0x23655e28 WebKit::WebViewImpl::paint(SkCanvas*, WebKit::WebRect const&, WebKit::WebWidget::PaintOptions) + 840 STDERR: 32 DumpRenderTree 0x22baa308 WebViewHost::paintRect(WebKit::WebRect const&) + 552 STDERR: 33 DumpRenderTree 0x22baa7c9 WebViewHost::paintInvalidatedRegion() + 745 STDERR: 34 DumpRenderTree 0x22b88493 TestShell::dump() + 2003 STDERR: 35 DumpRenderTree 0x22b87c92 TestShell::testFinished() + 82 STDERR: 36 DumpRenderTree 0x22b4465b DRTTestRunner::WorkQueue::processWorkSoon() + 187 STDERR: 37 DumpRenderTree 0x22b44cca DRTTestRunner::locationChangeDone() + 138 STDERR: 38 DumpRenderTree 0x22ba52d8 WebViewHost::locationChangeDone(WebKit::WebFrame*) + 88 STDERR: 39 DumpRenderTree 0x22ba5c63 WebViewHost::didFinishLoad(WebKit::WebFrame*) + 163 STDERR: 40 DumpRenderTree 0x22ba5cb9 non-virtual thunk to WebViewHost::didFinishLoad(WebKit::WebFrame*) + 41 STDERR: 41 libwebkit.dylib 0x234d3104 WebKit::FrameLoaderClientImpl::dispatchDidFinishLoad() + 148 STDERR: 42 libwebkit.dylib 0x25709ce9 WebCore::FrameLoader::checkLoadCompleteForThisFrame() + 1945 STDERR: 43 libwebkit.dylib 0x25700529 WebCore::FrameLoader::checkLoadComplete() + 425 STDERR: 44 libwebkit.dylib 0x256ffdd1 WebCore::FrameLoader::checkCompleted() + 433 STDERR: 45 libwebkit.dylib 0x256fffcb WebCore::FrameLoader::loadDone() + 43 STDERR: 46 libwebkit.dylib 0x257aa7bf WebCore::CachedResourceLoader::loadDone() + 127 STDERR: 47 libwebkit.dylib 0x25758008 WebCore::SubresourceLoader::releaseResources() + 216 STDERR: 48 libwebkit.dylib 0x2574e0a3 WebCore::ResourceLoader::didFinishLoading(double) + 99 STDERR: 49 libwebkit.dylib 0x25757adc WebCore::SubresourceLoader::didFinishLoading(double) + 716 STDERR: 50 libwebkit.dylib 0x2574eb0f WebCore::ResourceLoader::didFinishLoading(WebCore::ResourceHandle*, double) + 79 STDERR: 51 libwebkit.dylib 0x24ba2cda WebCore::ResourceHandleInternal::didFinishLoading(WebKit::WebURLLoader*, double) + 282 STDERR: 52 libglue.dylib 0x2ef8006e webkit_glue::WebURLLoaderImpl::Context::OnCompletedRequest(int, bool, std::string const&, base::TimeTicks const&) + 1134 STDERR: 53 DumpRenderTree 0x22e65c6d (anonymous namespace)::RequestProxy::NotifyCompletedRequest(int, std::string const&, base::TimeTicks const&) + 125 STDERR: 54 DumpRenderTree 0x22e66487 base::internal::RunnableAdapter<void ((anonymous namespace)::RequestProxy::*)(int, std::string const&, base::TimeTicks const&)>::Run((anonymous namespace)::RequestProxy*, int const&, std::string const&, base::TimeTicks const&) + 215 STDERR: 55 DumpRenderTree 0x22e66386 base::internal::InvokeHelper<false, void, base::internal::RunnableAdapter<void ((anonymous namespace)::RequestProxy::*)(int, std::string const&, base::TimeTicks const&)>, void ()((anonymous namespace)::RequestProxy* const&, int const&, std::string const&, base::TimeTicks const&)>::MakeItSo(base::internal::RunnableAdapter<void ((anonymous namespace)::RequestProxy::*)(int, std::string const&, base::TimeTicks const&)>, (anonymous namespace)::RequestProxy* const&, int const&, std::string const&, base::TimeTicks const&) + 150 STDERR: 56 DumpRenderTree 0x22e662c5 base::internal::Invoker<4, base::internal::BindState<base::internal::RunnableAdapter<void ((anonymous namespace)::RequestProxy::*)(int, std::string const&, base::TimeTicks const&)>, void ()((anonymous namespace)::RequestProxy*, int, std::string const&, base::TimeTicks const&), void ()((anonymous namespace)::RequestProxy*, int, std::string, base::TimeTicks)>, void ()((anonymous namespace)::RequestProxy*, int, std::string const&, base::TimeTicks const&)>::Run(base::internal::BindStateBase*) + 229 STDERR: 57 libbase.dylib 0x2c04ecdb base::Callback<void ()()>::Run() const + 75 STDERR: 58 libbase.dylib 0x2c0da147 MessageLoop::RunTask(base::PendingTask const&) + 1159 STDERR: 59 libbase.dylib 0x2c0da642 MessageLoop::DeferOrRunPendingTask(base::PendingTask const&) + 98 STDERR: 60 libbase.dylib 0x2c0da842 MessageLoop::DoWork() + 322 STDERR: 61 libbase.dylib 0x2c02d05b base::MessagePumpCFRunLoopBase::RunWork() + 107 STDERR: ax: bbadbeef, bx: 3, cx: 10eadfe0, dx: 10eadfe0 STDERR: di: 26283fe0, si: 26283f76, bp: bfffa2a8, sp: bfffa1b0, ss: 23, flags: 10282 STDERR: ip: 24a96b1e, cs: 1b, ds: 23, es: 23, fs: 0, gs: f

Re-opened since this is blocked by bug 99127

(In reply to comment #26) > Re-opened since this is blocked by bug 99127 It seems this patch can not cause "ASSERTION FAILED: m_samplerLocation != -1", but I'll fix the assertion fail and then post this patch again.

> It seems this patch can not cause "ASSERTION FAILED: m_samplerLocation != -1" Reverting the patch appears to have made the assertion go away.

(In reply to comment #28) > > It seems this patch can not cause "ASSERTION FAILED: m_samplerLocation != -1" > > Reverting the patch appears to have made the assertion go away. Ok. Could I know which test cases cause this crash. I could not reproduce crash using css1/filters/custom/*, although I tested using Qt.

I'm sorry, I forgot to record that information. :(

This assert means we can't find the sampler "css_u_texture" in the compiled program. Maybe the driver compiler optimizes away "css_u_texture" when the compositing mode is clear, since it sees a multiplication by zero?

(In reply to comment #31) > This assert means we can't find the sampler "css_u_texture" in the compiled program. Maybe the driver compiler optimizes away "css_u_texture" when the compositing mode is clear, since it sees a multiplication by zero? Yeah, good suggestion on where to start. Thanks for rolling it out Adam.

(In reply to comment #32) > (In reply to comment #31) > > This assert means we can't find the sampler "css_u_texture" in the compiled program. Maybe the driver compiler optimizes away "css_u_texture" when the compositing mode is clear, since it sees a multiplication by zero? > > Yeah, good suggestion on where to start. > > Thanks for rolling it out Adam. Yup, thanks Adam :)

(In reply to comment #30) > I'm sorry, I forgot to record that information. :( Ok. Thanks. I reproduced.

(In reply to comment #31) > This assert means we can't find the sampler "css_u_texture" in the compiled program. Maybe the driver compiler optimizes away "css_u_texture" when the compositing mode is clear, since it sees a multiplication by zero? Yes, exactly! When the compositing mode is clear or copy, the driver compiler optimizes may remove "css_u_texture", so we may remove assertions.

Created attachment 168535 [details] Patch

(In reply to comment #36) > Created an attachment (id=168535) [details] > Patch I just removed ASSERTIONs in CustomFilterCompiledProgram::initializeParameterLocations(). It is ok because FECustomFilter checks if uniform and attribute are -1 before binding them. 1. For ASSERT(m_samplerLocation != -1), FECustomFilter binds m_samplerLocation uniform if m_samplerLocation is not -1. void FECustomFilter::bindProgramAndBuffers(Platform3DObject inputTexture) { .... if (programNeedsInputTexture()) { .... m_context->activeTexture(GraphicsContext3D::TEXTURE0); m_context->uniform1i(m_compiledProgram->samplerLocation(), 0); m_context->bindTexture(GraphicsContext3D::TEXTURE_2D, inputTexture); .... } .... } bool FECustomFilter::programNeedsInputTexture() const { ASSERT(m_compiledProgram.get()); return m_compiledProgram->samplerLocation() != -1; } 2. For ASSERT(m_internalTexCoordAttribLocation != -1), FECustomFilter binds m_internalTexCoordAttribLocation attribute if m_internalTexCoordAttribLocation is not -1. void FECustomFilter::bindProgramAndBuffers(Platform3DObject inputTexture) { .... bindVertexAttribute(m_compiledProgram->internalTexCoordAttribLocation(), TexAttribSize, TexAttribOffset); .... } void FECustomFilter::bindVertexAttribute(int attributeLocation, unsigned size, unsigned offset) { if (attributeLocation != -1) { m_context->vertexAttribPointer(attributeLocation, size, GraphicsContext3D::FLOAT, false, m_mesh->bytesPerVertex(), offset); m_context->enableVertexAttribArray(attributeLocation); } }

Comment on attachment 168535 [details] Patch Thanks for investigating the issue! View in context: https://bugs.webkit.org/attachment.cgi?id=168535&action=review > Source/WebCore/ChangeLog:19 > + Remove ASSERTION checking if glGetUniformlocation returns negative, Nit: Capitalization "glGetUniformLocation". > Source/WebCore/ChangeLog:24 > + glGetuniformlocation returns -1. glGetAttribLocation ditto. Ditto. > Source/WebCore/platform/graphics/filters/CustomFilterCompiledProgram.cpp:-139 > - ASSERT(m_samplerLocation != -1); This assertion is nice because it can catch bugs in the shader rewriting, so I'm not sure we should remove it. Perhaps we can move this assertion to: CustomFilterValidatedProgram::compiledProgram() And it can look like this: ASSERT(m_samplerLocation != -1 || !needsInputTexture()); Calling a new method: bool CustomFilterValidatedProgram::needsInputTexture() const { return m_programInfo.programType == PROGRAM_TYPE_BLENDS_ELEMENT_TEXTURE && m_programInfo.compositeOperator != CompositeClear && m_programInfo.compositeOperator != CompositeCopy; } I'm suggesting moving it CustomFilterValidatedProgram because CustomFilterCompiledProgram doesn't know about compositing operators. It only knows about the GLSL shader strings it should to compile. What do you think?

(In reply to comment #38) > (From update of attachment 168535 [details]) > Thanks for investigating the issue! > This assertion is nice because it can catch bugs in the shader rewriting, so I'm not sure we should remove it. > > I'm suggesting moving it CustomFilterValidatedProgram because CustomFilterCompiledProgram doesn't know about compositing operators. It only knows about the GLSL shader strings it should to compile. > > What do you think? Thanks for your explanation! You are right absolutely. I wanted to not remove ASSERTIONs but I did not know how CustomFilterCompiledProgram can know about compositing operators. I'll update soon.

Created attachment 169042 [details] Patch

(In reply to comment #40) > Created an attachment (id=169042) [details] > Patch Move assertions in CustomFilterCompiledProgram to CustomFilterValidatedProgram as Max suggested.

(In reply to comment #41) > (In reply to comment #40) > > Created an attachment (id=169042) [details] [details] > > Patch > > Move assertions in CustomFilterCompiledProgram to CustomFilterValidatedProgram as Max suggested. Thanks! This looks good to me. Perhaps you can ask Dean for another look :)

Comment on attachment 169042 [details] Patch Clearing flags on attachment: 169042 Committed r132267: <http://trac.webkit.org/changeset/132267>

All reviewed patches have been landed. Closing bug.