Bug 92453

Summary: [chromium] factor out the optimization pass in CCRenderSurfaceFilters::apply
Product: WebKit Reporter: Antoine Labour <piman>
Component: New BugsAssignee: Antoine Labour <piman>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, cc-bugs, danakj, dglazkov, enne, fishd, jamesr, nduca, senorblanco, tkent+wkapi, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 92452    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from gce-cr-linux-02
none
Archive of layout-test-results from gce-cr-linux-05
none
Patch
none
Patch
none
Patch none

Description Antoine Labour 2012-07-26 21:16:20 PDT
[chromium] factor out the optimization pass in CCRenderSurfaceFilters::apply
Comment 1 Antoine Labour 2012-07-26 21:19:59 PDT
Created attachment 154830 [details]
Patch
Comment 2 Antoine Labour 2012-07-26 21:21:40 PDT
Let the bikeshedding begin.
Comment 3 Dana Jansens 2012-07-26 22:14:02 PDT
Comment on attachment 154830 [details]
Patch

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

> Source/Platform/chromium/src/WebFilterOperation.cpp:42
> +                && m_dropShadowOffset == other.m_dropShadowOffset
> +                && m_dropShadowColor == other.m_dropShadowColor;

quick readthru style nit: indent only 4 spaces

> Source/WebKit/chromium/tests/CCRenderSurfaceFiltersTest.cpp:2
> + * Copyright (C) 2011 Google Inc. All rights reserved.

2012
Comment 4 WebKit Review Bot 2012-07-26 23:58:06 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 5 WebKit Review Bot 2012-07-27 03:52:18 PDT
Comment on attachment 154830 [details]
Patch

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

New failing tests:
CCRenderSurfaceFiltersTest.testColorMatrixFiltersCombined
Comment 6 WebKit Review Bot 2012-07-27 03:52:23 PDT
Created attachment 154898 [details]
Archive of layout-test-results from gce-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 7 WebKit Review Bot 2012-07-27 04:33:54 PDT
Comment on attachment 154830 [details]
Patch

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

New failing tests:
CCRenderSurfaceFiltersTest.testColorMatrixFiltersCombined
Comment 8 WebKit Review Bot 2012-07-27 04:33:59 PDT
Created attachment 154904 [details]
Archive of layout-test-results from gce-cr-linux-05

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 9 Antoine Labour 2012-07-27 14:57:46 PDT
Created attachment 155059 [details]
Patch
Comment 10 Antoine Labour 2012-07-27 15:03:42 PDT
(In reply to comment #3)
> (From update of attachment 154830 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154830&action=review
> 
> > Source/Platform/chromium/src/WebFilterOperation.cpp:42
> > +                && m_dropShadowOffset == other.m_dropShadowOffset
> > +                && m_dropShadowColor == other.m_dropShadowColor;
> 
> quick readthru style nit: indent only 4 spaces

Done.

> 
> > Source/WebKit/chromium/tests/CCRenderSurfaceFiltersTest.cpp:2
> > + * Copyright (C) 2011 Google Inc. All rights reserved.
> 
> 2012

Done.

Note: the failing unittests are because of https://bugs.webkit.org/show_bug.cgi?id=92452 not having landed yet.
Comment 11 Stephen White 2012-07-30 08:38:21 PDT
Comment on attachment 155059 [details]
Patch

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

> Source/Platform/chromium/public/WebFilterOperation.h:94
> +    float m_matrix[20];

This should probably be SkScalar (and elsewhere).

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceFilters.cpp:275
> +        int scratchCount = std::min(2, filterCount);

Why the min?  Shouldn't a filter chain of length 1 only need a single buffer?

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceFilters.cpp:394
> +    state.init(optimizedFilters.size());

I think you should be checking the return value here and doing an early-out on failure.
Comment 12 Stephen White 2012-07-30 08:39:14 PDT
(In reply to comment #11)
> (From update of attachment 155059 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=155059&action=review
> 
> > Source/Platform/chromium/public/WebFilterOperation.h:94
> > +    float m_matrix[20];
> 
> This should probably be SkScalar (and elsewhere).
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceFilters.cpp:275
> > +        int scratchCount = std::min(2, filterCount);
> 
> Why the min?  Shouldn't a filter chain of length 1 only need a single buffer?

Ignore that.  I have Monday brain.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceFilters.cpp:394
> > +    state.init(optimizedFilters.size());
> 
> I think you should be checking the return value here and doing an early-out on failure.
Comment 13 Antoine Labour 2012-07-30 20:17:45 PDT
Created attachment 155429 [details]
Patch
Comment 14 Antoine Labour 2012-07-30 20:20:05 PDT
(In reply to comment #11)
> (From update of attachment 155059 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=155059&action=review
> 
> > Source/Platform/chromium/public/WebFilterOperation.h:94
> > +    float m_matrix[20];
> 
> This should probably be SkScalar (and elsewhere).

Done.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceFilters.cpp:275
> > +        int scratchCount = std::min(2, filterCount);
> 
> Why the min?  Shouldn't a filter chain of length 1 only need a single buffer?
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCRenderSurfaceFilters.cpp:394
> > +    state.init(optimizedFilters.size());
> 
> I think you should be checking the return value here and doing an early-out on failure.

Done - that's what I meant, I just forgot to write the code :/
Comment 15 Antoine Labour 2012-08-01 15:45:12 PDT
Ping?

James & all, any comments about the WebFilterOperation API addition?
Comment 16 James Robinson 2012-08-01 15:51:37 PDT
Seems fine - IIUC Stephen White wants to rewrite this soon anyway, so it may all go away.
Comment 17 James Robinson 2012-08-01 15:54:23 PDT
Comment on attachment 155429 [details]
Patch

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

> Source/Platform/Platform.gyp/Platform.gyp:39
> +            # Location of the chromium src directory and target type is different
> +            # if webkit is built inside chromium or as standalone project.

skia should be available at <(DEPTH)/skia/... no matter what sort of checkout we're in.  In a full chromium checkout, <(DEPTH) is src/ and skia is at src/skia/.... In a WebKit standalone, <(DEPTH) is Source/WebKit/chromium and skia/ is pulled in to Source/WebKit/chromium/skia/...
Comment 18 Antoine Labour 2012-08-01 16:20:34 PDT
Created attachment 155917 [details]
Patch
Comment 19 Antoine Labour 2012-08-01 16:21:29 PDT
(In reply to comment #17)
> (From update of attachment 155429 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=155429&action=review
> 
> > Source/Platform/Platform.gyp/Platform.gyp:39
> > +            # Location of the chromium src directory and target type is different
> > +            # if webkit is built inside chromium or as standalone project.
> 
> skia should be available at <(DEPTH)/skia/... no matter what sort of checkout we're in.  In a full chromium checkout, <(DEPTH) is src/ and skia is at src/skia/.... In a WebKit standalone, <(DEPTH) is Source/WebKit/chromium and skia/ is pulled in to Source/WebKit/chromium/skia/...

Ok, done.
Comment 20 James Robinson 2012-08-01 16:45:32 PDT
Comment on attachment 155917 [details]
Patch

R=me
Comment 21 WebKit Review Bot 2012-08-01 17:22:21 PDT
Comment on attachment 155917 [details]
Patch

Clearing flags on attachment: 155917

Committed r124392: <http://trac.webkit.org/changeset/124392>
Comment 22 WebKit Review Bot 2012-08-01 17:22:27 PDT
All reviewed patches have been landed.  Closing bug.