Bug 107712

Summary: [Skia] Native linear gradient tiling
Product: WebKit Reporter: Florin Malita <fmalita>
Component: Layout and RenderingAssignee: Florin Malita <fmalita>
Status: RESOLVED WONTFIX    
Severity: Normal CC: buildbot, darin, gyuyoung.kim, junov, laszlo.gombos, ostap73, pdr, rakuco, reed, rniwa, robertphillips, schenney, senorblanco, webkit-ews, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch buildbot: commit-queue-

Florin Malita
Reported 2013-01-23 11:51:10 PST
Currently, GeneratorGeneratedImage::drawPattern() creates a tile image buffer which is drawn repeatedly to fill the target area. As an optimization, for axis-aligned linear gradients, the image buffer only holds a 1px slice of the gradient which is then re-sampled to the tile size when drawn. The drawPatern() path is quite common in practice (for example painting the background for boxes with a border), so it would be great if we could avoid rasterizing/resampling at this level, and simply pass the gradient info to Skia instead. Skia cannot handle gradient tiling in general, but for axis aligned linear gradients (presumably the most common case) we can emulate tiling by setting a surrogate gradient up in the following manner: * set the start/stop (p0/p1) to match the tile edges * filter out color stops that fall outside the tile range, and adjust offsets for the remaining ones relative to the new normalized (p0,p1) interval * for the first (0.0) and last (1.0) color stops (corresponding to the tile edges), use interpolates values from the original gradient This should produce equivalent results when drawn across the target area - the difference being we now perform a single fill operation and Skia handles the gradient repeat. I'll share a patch shortly.
Attachments
Patch (78.97 KB, patch)
2013-01-23 14:22 PST, Florin Malita 		no flags
DetailsFormatted DiffDiff
Patch (79.30 KB, patch)
2013-01-23 14:45 PST, Florin Malita 		no flags
DetailsFormatted DiffDiff
Patch (79.36 KB, patch)
2013-01-24 13:25 PST, Florin Malita 		no flags
DetailsFormatted DiffDiff
Patch (79.25 KB, patch)
2013-02-07 06:47 PST, Florin Malita 		no flags
DetailsFormatted DiffDiff
Patch (88.14 KB, patch)
2013-02-08 08:53 PST, Florin Malita 		buildbot: commit-queue-
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Florin Malita
Comment 1 2013-01-23 14:22:59 PST
Created attachment 184312 [details] Patch First pass.
WebKit Review Bot
Comment 2 2013-01-23 14:26:13 PST
Attachment 184312 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/gradients/linear-tiled-gradients.html', u'LayoutTests/platform/chromium-linux/fast/backgrounds/gradient-background-leakage-2-expected.png', u'LayoutTests/platform/chromium-linux/fast/backgrounds/gradient-background-leakage-expected.png', u'LayoutTests/platform/chromium-linux/fast/gradients/border-image-gradient-expected.png', u'LayoutTests/platform/chromium-linux/fast/gradients/border-image-gradient-sides-and-corners-expected.png', u'LayoutTests/platform/chromium-linux/fast/gradients/linear-tiled-gradients-expected.png', u'LayoutTests/platform/chromium-linux/fast/gradients/linear-tiled-gradients-expected.txt', u'LayoutTests/platform/chromium-linux/fast/gradients/simple-gradients-expected.png', u'LayoutTests/platform/chromium/TestExpectations', u'LayoutTests/platform/efl/TestExpectations', u'LayoutTests/platform/gtk/TestExpectations', u'LayoutTests/platform/mac/TestExpectations', u'LayoutTests/platform/qt/TestExpectations', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/Generator.h', u'Source/WebCore/platform/graphics/GeneratorGeneratedImage.cpp', u'Source/WebCore/platform/graphics/GeneratorGeneratedImage.h', u'Source/WebCore/platform/graphics/Gradient.cpp', u'Source/WebCore/platform/graphics/Gradient.h', u'Source/WebCore/platform/graphics/skia/GradientSkia.cpp']" exit_code: 1 LayoutTests/platform/chromium-linux/fast/gradients/simple-gradients-expected.png:0: Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png"). [image/png] [5] Total errors found: 1 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.
Early Warning System Bot
Comment 3 2013-01-23 14:32:28 PST
Comment on attachment 184312 [details] Patch Attachment 184312 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/16077343
Florin Malita
Comment 4 2013-01-23 14:45:33 PST
Created attachment 184316 [details] Patch Fix the non-Skia builds.
WebKit Review Bot
Comment 5 2013-01-23 14:47:31 PST
Attachment 184316 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/gradients/linear-tiled-gradients.html', u'LayoutTests/platform/chromium-linux/fast/backgrounds/gradient-background-leakage-2-expected.png', u'LayoutTests/platform/chromium-linux/fast/backgrounds/gradient-background-leakage-expected.png', u'LayoutTests/platform/chromium-linux/fast/gradients/border-image-gradient-expected.png', u'LayoutTests/platform/chromium-linux/fast/gradients/border-image-gradient-sides-and-corners-expected.png', u'LayoutTests/platform/chromium-linux/fast/gradients/linear-tiled-gradients-expected.png', u'LayoutTests/platform/chromium-linux/fast/gradients/linear-tiled-gradients-expected.txt', u'LayoutTests/platform/chromium-linux/fast/gradients/simple-gradients-expected.png', u'LayoutTests/platform/chromium/TestExpectations', u'LayoutTests/platform/efl/TestExpectations', u'LayoutTests/platform/gtk/TestExpectations', u'LayoutTests/platform/mac/TestExpectations', u'LayoutTests/platform/qt/TestExpectations', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/Generator.h', u'Source/WebCore/platform/graphics/GeneratorGeneratedImage.cpp', u'Source/WebCore/platform/graphics/GeneratorGeneratedImage.h', u'Source/WebCore/platform/graphics/Gradient.cpp', u'Source/WebCore/platform/graphics/Gradient.h', u'Source/WebCore/platform/graphics/skia/GradientSkia.cpp']" exit_code: 1 LayoutTests/platform/chromium-linux/fast/gradients/simple-gradients-expected.png:0: Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png"). [image/png] [5] Total errors found: 1 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.
Florin Malita
Comment 6 2013-01-24 13:25:33 PST
Created attachment 184568 [details] Patch Trying again just to see whether the check-webkit-style error was transient/bot related.
WebKit Review Bot
Comment 7 2013-01-24 13:30:19 PST
Attachment 184568 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/gradients/linear-tiled-gradients.html', u'LayoutTests/platform/chromium-linux/fast/backgrounds/gradient-background-leakage-2-expected.png', u'LayoutTests/platform/chromium-linux/fast/backgrounds/gradient-background-leakage-expected.png', u'LayoutTests/platform/chromium-linux/fast/gradients/border-image-gradient-expected.png', u'LayoutTests/platform/chromium-linux/fast/gradients/border-image-gradient-sides-and-corners-expected.png', u'LayoutTests/platform/chromium-linux/fast/gradients/linear-tiled-gradients-expected.png', u'LayoutTests/platform/chromium-linux/fast/gradients/linear-tiled-gradients-expected.txt', u'LayoutTests/platform/chromium-linux/fast/gradients/simple-gradients-expected.png', u'LayoutTests/platform/chromium/TestExpectations', u'LayoutTests/platform/efl/TestExpectations', u'LayoutTests/platform/gtk/TestExpectations', u'LayoutTests/platform/mac/TestExpectations', u'LayoutTests/platform/qt/TestExpectations', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/Generator.h', u'Source/WebCore/platform/graphics/GeneratorGeneratedImage.cpp', u'Source/WebCore/platform/graphics/GeneratorGeneratedImage.h', u'Source/WebCore/platform/graphics/Gradient.cpp', u'Source/WebCore/platform/graphics/Gradient.h', u'Source/WebCore/platform/graphics/skia/GradientSkia.cpp']" exit_code: 1 LayoutTests/platform/chromium-linux/fast/gradients/simple-gradients-expected.png:0: Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png"). [image/png] [5] Total errors found: 1 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.
Florin Malita
Comment 8 2013-01-24 13:36:32 PST
(In reply to comment #7) > LayoutTests/platform/chromium-linux/fast/gradients/simple-gradients-expected.png:0: Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png"). [image/png] [5] > Total errors found: 1 in 16 files Not sure what to make of this - I definitely have auto-props enabled and check-webkit-style passes locally: fmalita@fmalita-linux2:~/wk$ Tools/Scripts/check-webkit-style Total errors found: 0 in 16 files fmalita@fmalita-linux2:~/wk$ git svn propget "svn:mime-type" LayoutTests/platform/chromium-linux/fast/gradients/simple-gradients-expected.png image/png Since the tool is looking at the local svn config, I'm guessing this is either a check-webkit-style bug or bot misconfiguration.
Justin Novosad
Comment 9 2013-02-06 13:22:31 PST
(In reply to comment #0) > Currently, GeneratorGeneratedImage::drawPattern() creates a tile image buffer which is drawn repeatedly to fill the target area. As an optimization, for axis-aligned linear gradients, the image buffer only holds a 1px slice of the gradient which is then re-sampled to the tile size when drawn. I see a problem with this: Taking a 1px slice could result in stripe artifacts due to dithering. Any way to make the tile generator produce 2px slices instead?
Mike Reed
Comment 10 2013-02-07 06:03:25 PST
I believe the patch is meant to use skia's native gradient drawing more, and use constructed-bitmaps less. The result should be - better behavior/utilization w/ pictures - better or equivalent quality - better or equivalent speed
Robert Phillips
Comment 11 2013-02-07 06:09:16 PST
Skia currently has an optimized path for 1xN textures. Making them 2xN would block this fast path but, as Mike said, this CL is all about not even converting to a 1xN texture in the first place.
Justin Novosad
Comment 12 2013-02-07 06:15:15 PST
(In reply to comment #10) > I believe the patch is meant to use skia's native gradient drawing more, and use constructed-bitmaps less. The result should be > - better behavior/utilization w/ pictures > - better or equivalent quality > - better or equivalent speed After a closer look at the code, I tend to agree. The description was misleading. If a 1px slice ends up screwing with dithering, it would be skia's fault.
Florin Malita
Comment 13 2013-02-07 06:47:54 PST
Created attachment 187093 [details] Patch Rebased. Hopefully the style bot has also come to its senses.
Florin Malita
Comment 14 2013-02-08 08:53:40 PST
Created attachment 187329 [details] Patch Updated after the Skia flags changes.
Build Bot
Comment 15 2013-02-08 10:19:11 PST
Comment on attachment 187329 [details] Patch Attachment 187329 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16434703 New failing tests: http/tests/cache/cached-main-resource.html
Darin Adler
Comment 16 2013-04-09 09:40:07 PDT
Comment on attachment 187329 [details] Patch Clearing flags on Skia-specific patch.
Stephen Chenney
Comment 17 2013-04-09 10:52:01 PDT
Please don't WontFix bugs until we've had a chance to migrate them. It will only be a few days at most before we've finished that.
Darin Adler
Comment 18 2013-04-09 12:05:29 PDT
Sorry, I have no idea what you are talking about. I don’t know what migrating is, how to tell if a bug is migrated, or why marking them as RESOLVED WONTFIX gets in the way of migration. Could you post some instructions about this somewhere?
Stephen Chenney
Comment 19 2013-04-09 12:47:31 PDT
(In reply to comment #18) > Sorry, I have no idea what you are talking about. I don’t know what migrating is, how to tell if a bug is migrated, or why marking them as RESOLVED WONTFIX gets in the way of migration. Could you post some instructions about this somewhere? Sorry for not making things clearer. There is a chain on WebKit dev "Chromium bugs marked as WontFix" but it is not explicit about the procedure. We were trying to minimize the impact on WebKit developers - you shouldn't have to spend time helping us close out our bugs. The goal is to have no unresolved Chromium-only issues in WebKit while not losing any information for Chrome. To achieve that, we (mostly I) are searching for all unresolved bugs containing "Chromium" or "Skia" and using a Google internal tool to migrate them to crbug. It will take some time as we have no automagical way of deciding if a bug really needs to move across. Maybe a few days. I can't post the tool because it's a Chrome extension on the internal network. Believe me, I wish I could get more people involved. The only way we can avoid losing track of bugs is if they remain unresolved while we look at them. There's no way we can check resolved issues - there are too many. To speed things along I am doing absolutely minimal modifications to the WebKit bugs. The only way to see if a bug has been migrated is to go to crbug.com and search for "label:WebKit-ID-XXXXX" where XXXXX is the WebKit bug number. Email has also gone out to all those people with WebKit bugs related to Chromium that have a patch pending review. We will be doing what you're doing (clearing flags) as soon as we can safely do so.
Note You need to log in before you can comment on or make changes to this bug.