This change takes advantage of tiling (implemented on ShadowBlur::drawRectShadow) to improve drawing performance. However there are some issues to consider before finishing this change: [1] Both Qt and Cairo does not support tiling drawing on rotated rectangles (see bug 45042) - basically the shadows are painted well, but with some graphical artifacts between tile edges and sides. [2] However it works fine in Cairo, using drawRectShadow without tiling for rotated rectangles is currently broken on Qt (seems that shadow drawing does not respect rectangle rotation). [3] When forcing usage of tiling drawing code inside drawRectShadow on Qt port, issue [2] ceases to exist, but issue [1] maintains, so it is imperative to fix tiling artifacts and Qt handling of drawRectShadow before anything else. My plan is to let a provisional patch here for consideration, while working on the issues mentioned above. What do you guys think?
[1] Is a different bug. Should not be treated in this patch. For [2] and [3] would be great!
Created attachment 149771 [details] Proposed patch for Qt's GC This is the proposed patch for Qt's GC implementation. I haven't touched on the "void GraphicsContext::fillRect(const FloatRect& rect)" overloaded function as I haven't found any layout test that actually triggers it yet.
Changing bug summary to a proper meaning - less cryptful, at least :D
Created attachment 149792 [details] Proposed patch (with fix for shadow rotation) Updated patch with fix for rotated shadows in Qt's GraphicsContext::pushTransparencyLayerInternal. A missing device transform was preventing the shadows from being properly drawn. This patch probably will need some layout tests rebaselines :)
Created attachment 149934 [details] Proposed patch (v3) Fixed an unnecessary cast to QRectF on GraphicsContext::fillRoundedRect.
Comment on attachment 149934 [details] Proposed patch (v3) Clearing flags on attachment: 149934 Committed r121435: <http://trac.webkit.org/changeset/121435>
All reviewed patches have been landed. Closing bug.
Created attachment 150165 [details] Additional patch for GraphicsContext::fillRect(const FloatRect& rect) overloaded function This is an additional patch for a certain case in the first fillRect() overload. Tested with canvas/philip/tests/2d.shadow.blur.high.html and canvas/philip/tests/2d.shadow.blur.low.html with same results.
Changing status to "reopened" while additional patch is pending review/commit-queue authorization.
Comment on attachment 150165 [details] Additional patch for GraphicsContext::fillRect(const FloatRect& rect) overloaded function Clearing flags on attachment: 150165 Committed r121569: <http://trac.webkit.org/changeset/121569>
I believe this broke the Qt Linux release bot. Mind taking a look?
Reopening to attach new patch.
Created attachment 150271 [details] Patch for landing
Sure, sorry for the delay (just checked my mail - Manaus timezone is -04:00). I am running a second pass with and without the patch to see any changes on the test results. (In reply to comment #12) > I believe this broke the Qt Linux release bot. Mind taking a look?
Comment on attachment 150271 [details] Patch for landing Clearing flags on attachment: 150271 Committed r121618: <http://trac.webkit.org/changeset/121618>
Indeed, I detected 29 regressions with the additional patch. Sorry :/ I've only tested them with philip patches but forgot to do a full test run. Thanks for pointing it out, No'am.
Actually pardon again, but these 29 were actually tests with no expected results from the reverted patch test run, not the patched test run. The actual diff are these two tests: fast/canvas/canvas-scale-fillRect-shadow.html fast/canvas/canvas-transforms-fillRect-shadow.html I'll have a look at these tomorrow. (In reply to comment #18) > Indeed, I detected 29 regressions with the additional patch. Sorry :/ I've only tested them with philip patches but forgot to do a full test run. Thanks for pointing it out, No'am.
Seems that ShadowBlur's current drawRectShadow() implementation does not take into consideration Qt's brush properties for some reason. I am going to issue that into another bug. With the first patch landed and sounding well on tests, I'd assume this bug is fixed. (In reply to comment #19) > Actually pardon again, but these 29 were actually tests with no expected results from the reverted patch test run, not the patched test run. The actual diff are these two tests: > > fast/canvas/canvas-scale-fillRect-shadow.html > fast/canvas/canvas-transforms-fillRect-shadow.html > > I'll have a look at these tomorrow. > > (In reply to comment #18) > > Indeed, I detected 29 regressions with the additional patch. Sorry :/ I've only tested them with philip patches but forgot to do a full test run. Thanks for pointing it out, No'am.
The fix was rolled out, so the issue is still open.
Created attachment 187626 [details] Patch Fix for patch 'Additional patch for GraphicsContext::fillRect(const FloatRect& rect) overloaded function' which was rolled out in comment #16.
Comment on attachment 187626 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=187626&action=review > Source/WebCore/ChangeLog:9 > + This overloaded fillRect implementation also supports this optimization in > + certain situations. Could you instead explain what the issue was and what the patch changes in ShadowBlur?
(In reply to comment #23) > (From update of attachment 187626 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=187626&action=review > > > Source/WebCore/ChangeLog:9 > > + This overloaded fillRect implementation also supports this optimization in > > + certain situations. > > Could you instead explain what the issue was and what the patch changes in ShadowBlur? The main goal of this patch is improving performance according to what is written in comment #1. I am not 100% sure if the changes I made in the patch are correct, that is why I asked for reviewing. One thing I noticed during my tests is that using save()/restore() instead of GraphicsContextStateSaver in computeSliceSizesFromRadii() helps fixing the regressions mentioned in comment #19. The changes in ShadowBlur::drawShadowBuffer() and computeSliceSizesFromRadii() are basically to fix the regressions but I am not sure exactly how this code work.
This patch is a working in progress. What I did was compare the sequence of function/method calls with and without the original patch (https://bugs.webkit.org/attachment.cgi?id=149934). My patch basically outlines the difference between the two sequence of calls. I still need to investigate further why the changes in ShadowBlur::drawShadowBuffer() makes the regressions mentioned in comment #19 go away. The main goal is to use ShadowBlur::drawRectShadow(), which according to the bug description, improves drawing performance by taking advantage of tiled shadow rendering.
Created attachment 192473 [details] Patch
(In reply to comment #24) > (In reply to comment #23) > > (From update of attachment 187626 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=187626&action=review > > > > > Source/WebCore/ChangeLog:9 > > > + This overloaded fillRect implementation also supports this optimization in > > > + certain situations. > > > > Could you instead explain what the issue was and what the patch changes in ShadowBlur? > > The main goal of this patch is improving performance according to what is written in comment #1. I am not 100% sure if the changes I made in the patch are correct, that is why I asked for reviewing. One thing I noticed during my tests is that using save()/restore() instead of GraphicsContextStateSaver in computeSliceSizesFromRadii() helps fixing the regressions mentioned in comment #19. The changes in ShadowBlur::drawShadowBuffer() and computeSliceSizesFromRadii() are basically to fix the regressions but I am not sure exactly how this code work. Your patch only worked on the transform test-case not the scale test-case, but it worked by avoiding clipToImageBuffer which produces scaling artifacts due to the way we transform alphaMaps.
Committed r145810: <http://trac.webkit.org/changeset/145810>