Bug 92453 – [chromium] factor out the optimization pass in CCRenderSurfaceFilters::apply

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Antoine Labour]

[chromium] factor out the optimization pass in CCRenderSurfaceFilters::apply

[Antoine Labour]

Created attachment 154830 [details]
Patch

[Antoine Labour]

Let the bikeshedding begin.

[Dana Jansens]

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

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Antoine Labour]

Created attachment 155059 [details]
Patch

[Antoine Labour]

(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.

[Stephen White]

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.

[Stephen White]

(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.

[Antoine Labour]

Created attachment 155429 [details]
Patch

[Antoine Labour]

(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 :/

[Antoine Labour]

Ping?

James & all, any comments about the WebFilterOperation API addition?

[James Robinson]

Seems fine - IIUC Stephen White wants to rewrite this soon anyway, so it may all go away.

[James Robinson]

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/...

[Antoine Labour]

Created attachment 155917 [details]
Patch

[Antoine Labour]

(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.

[James Robinson]

Comment on attachment 155917 [details]
Patch

R=me

[WebKit Review Bot]

Comment on attachment 155917 [details]
Patch

Clearing flags on attachment: 155917

Committed r124392: <http://trac.webkit.org/changeset/124392>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.