Bug 76026 – [Texmap] Support filters in TextureMapperImageBuffer

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Noam Rosenthal]

Allow rendering of CSS filters when in compositing mode.

[Noam Rosenthal]

Created attachment 121981 [details]
Patch

[Simon Hausmann]

Comment on attachment 121981 [details]
Patch

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

> Source/WebCore/platform/graphics/qt/TextureMapperQt.cpp:204
> +typedef QRgb (*PixelFilterFunction)(QRgb, const double&);

Is there an advantage/reason for passing const double& when many the calling conventions on many architectures allow for passing of doubles in registers? (SSE or integer register pair on ARM IIRC)

> Source/WebCore/platform/graphics/qt/TextureMapperQt.cpp:316
> +    static const PixelFilterFunction functions[] = {0, grayscale, sepia, saturate, hueRotate, invert, opacity, brightness, contrast };

Coding style: missing space after {

> Source/WebCore/platform/graphics/qt/TextureMapperQt.cpp:317
> +    PixelFilterFunction function = functions[type];

Perhaps it would make sense to have an ASSERT to ensure that type stays within the bounds of the functions array, as FilterOperation::OperationType may be re-ordered at any time.

[Noam Rosenthal]

> Is there an advantage/reason for passing const double& when many the calling conventions on many architectures allow for passing of doubles in registers? (SSE or integer register pair on ARM IIRC)

Was following same style as in the software implementation of filters... I'll revise.

> > Source/WebCore/platform/graphics/qt/TextureMapperQt.cpp:317
> > +    PixelFilterFunction function = functions[type];
> 
> Perhaps it would make sense to have an ASSERT to ensure that type stays within the bounds of the functions array, as FilterOperation::OperationType may be re-ordered at any time.
OK.

[Kenneth Rohde Christiansen]

Comment on attachment 121981 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Also, added a fully working software implementation in the Qt backend of TextureMapper.r

spelling issue at the end

> Source/WebCore/platform/graphics/qt/TextureMapperQt.cpp:215
> +    return qRgba(
> +        red * (0.2126 + 0.7874 * (1. - amount)) + green * (0.7152 - 0.7152 * (1. - amount)) + blue * (0.0722 - 0.0722 * (1. - amount)),
> +        red * (0.2126 - 0.2126 * (1. - amount)) + green * (0.7152 + 0.2848 * (1. - amount)) + blue * (0.0722 - 0.0722 * (1. - amount)),
> +        red * (0.2126 - 0.2126 * (1. - amount)) + green * (0.7152 - 0.7152 * (1. - amount)) + blue * (0.0722 + 0.9278 * (1. - amount)),
> +        qAlpha(color)

any comment about what these numbers represents?

> Source/WebCore/platform/graphics/qt/TextureMapperQt.cpp:360
> +// This is a "trick" algorithm for fast gaussian blur. It's not as accurate as FEGaussianBlur,
> +// but can be potentially faster.
> +static void quickBlur(QImage& image, const double& stdDeviation)

Maybe this is not the right place for it then?

> Source/WebCore/platform/graphics/texmap/TextureMapperFilterRenderer.cpp:2
> + Copyright (C) 2010 Nokia Corporation and/or its subsidiary(-ies)

Old code :) 2012 right?

[Noam Rosenthal]

(In reply to comment #4)
> any comment about what these numbers represents?
Will do. They're taken directly from the W3C filters spec.

> 
> > Source/WebCore/platform/graphics/qt/TextureMapperQt.cpp:360
> > +// This is a "trick" algorithm for fast gaussian blur. It's not as accurate as FEGaussianBlur,
> > +// but can be potentially faster.
> > +static void quickBlur(QImage& image, const double& stdDeviation)
> 
> Maybe this is not the right place for it then?
Not sure what you mean.

[Noam Rosenthal]

Comment on attachment 121981 [details]
Patch

Removing review flags until all of the texture-mapper refactors are in.

[Noam Rosenthal]

Created attachment 127174 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
Index mismatch: 883c7fb9c78a1d23f9131b69f7fa908cd9778764 != 1fd67dff9ac4ff9ce56307043a73020a333c7b89
rereading 35ee22447c69292de5be6ed45c2410bff7ce3dca
	M	LayoutTests/platform/chromium-linux/tables/mozilla/bugs/bug11026-expected.png
	M	LayoutTests/platform/chromium-linux/tables/mozilla/bugs/bug10565-expected.png
	M	LayoutTests/platform/chromium-linux/tables/mozilla/bugs/bug1188-expected.png
	M	LayoutTests/platform/chromium/test_expectations.txt
	A	LayoutTests/platform/chromium-mac-snowleopard/tables/mozilla/bugs/bug11026-expected.png
	A	LayoutTests/platform/chromium-mac-snowleopard/tables/mozilla/bugs/bug10565-expected.png
	M	LayoutTests/platform/chromium-mac-snowleopard/tables/mozilla/bugs/bug1188-expected.png
	D	LayoutTests/platform/chromium-mac/tables/mozilla/bugs/bug11026-expected.png
	D	LayoutTests/platform/chromium-mac/tables/mozilla/bugs/bug10565-expected.png
	M	LayoutTests/platform/chromium-mac-leopard/tables/mozilla/bugs/bug11026-expected.png
	M	LayoutTests/platform/chromium-mac-leopard/tables/mozilla/bugs/bug10565-expected.png
	M	LayoutTests/platform/chromium-mac-leopard/tables/mozilla/bugs/bug1188-expected.png
	M	LayoutTests/platform/chromium-win/tables/mozilla/bugs/bug1188-expected.png
	M	LayoutTests/platform/chromium-win/tables/mozilla/bugs/bug11026-expected.png
	M	LayoutTests/platform/chromium-win/tables/mozilla/bugs/bug10565-expected.png
	M	LayoutTests/ChangeLog
W: -empty_dir: trunk/LayoutTests/platform/chromium-mac/tables/mozilla/bugs/bug10565-expected.png
W: -empty_dir: trunk/LayoutTests/platform/chromium-mac/tables/mozilla/bugs/bug11026-expected.png
107807 = cd492113759b3acf3e51434935a37ee8efc9ac66 already exists! Why are we refetching it?
 at /usr/lib/git-core/git-svn line 5210

Died at Tools/Scripts/update-webkit line 164.


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

[WebKit Review Bot]

Comment on attachment 127174 [details]
Patch

Clearing flags on attachment: 127174

Committed r107814: <http://trac.webkit.org/changeset/107814>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

I had to update the expected results - http://trac.webkit.org/changeset/107898

I assume you committed Mac results, but the reference platform is Linux, not Mac. :(