Bug 76026 - [Texmap] Support filters in TextureMapperImageBuffer
Summary: [Texmap] Support filters in TextureMapperImageBuffer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Noam Rosenthal
URL:
Keywords: Qt
Depends on:
Blocks: 75918 75677
  Show dependency treegraph
 
Reported: 2012-01-10 21:28 PST by Noam Rosenthal
Modified: 2012-02-15 23:40 PST (History)
5 users (show)

See Also:


Attachments
Patch (820.92 KB, patch)
2012-01-10 23:06 PST, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (327.09 KB, patch)
2012-02-15 06:27 PST, Noam Rosenthal
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Noam Rosenthal 2012-01-10 21:28:13 PST
Allow rendering of CSS filters when in compositing mode.
Comment 1 Noam Rosenthal 2012-01-10 23:06:43 PST
Created attachment 121981 [details]
Patch
Comment 2 Simon Hausmann 2012-01-11 07:52:28 PST
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.
Comment 3 Noam Rosenthal 2012-01-11 09:06:03 PST
> 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.
Comment 4 Kenneth Rohde Christiansen 2012-01-26 15:23:27 PST
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?
Comment 5 Noam Rosenthal 2012-01-26 15:56:21 PST
(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.
Comment 6 Noam Rosenthal 2012-01-26 16:36:02 PST
Comment on attachment 121981 [details]
Patch

Removing review flags until all of the texture-mapper refactors are in.
Comment 7 Noam Rosenthal 2012-02-15 06:27:56 PST
Created attachment 127174 [details]
Patch
Comment 8 WebKit Review Bot 2012-02-15 06:31:29 PST
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.
Comment 9 WebKit Review Bot 2012-02-15 07:30:06 PST
Comment on attachment 127174 [details]
Patch

Clearing flags on attachment: 127174

Committed r107814: <http://trac.webkit.org/changeset/107814>
Comment 10 WebKit Review Bot 2012-02-15 07:30:24 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 Csaba Osztrogonác 2012-02-15 23:40:29 PST
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. :(