Summary: | [Qt] Add support for tiled shadow blur | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Bruno Abinader (history only) <bruno.abinader> | ||||||||||||||||
Component: | WebKit Qt | Assignee: | Lamarque V. Souza <lamarque> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Enhancement | CC: | allan.jensen, hausmann, helder.correia, igor.oliveira, kenneth, noam, webkit.review.bot | ||||||||||||||||
Priority: | P2 | Keywords: | Qt | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Bug Depends on: | 45042, 90562, 111736 | ||||||||||||||||||
Bug Blocks: | |||||||||||||||||||
Attachments: |
|
Description
Bruno Abinader (history only)
2012-06-27 10:04:16 PDT
[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> All reviewed patches have been landed. Closing bug. 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> All reviewed patches have been landed. Closing bug. 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> |