Bug 90082 – [Qt] Add support for tiled shadow blur

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Bruno Abinader (history only)]

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?

[Igor Trindade Oliveira]

[1] Is a different bug. Should not be treated in this patch.

For [2] and [3] would be great!

[Bruno Abinader (history only)]

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.

[Bruno Abinader (history only)]

Changing bug summary to a proper meaning - less cryptful, at least :D

[Bruno Abinader (history only)]

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 :)

[Bruno Abinader (history only)]

Created attachment 149934 [details]
Proposed patch (v3)

Fixed an unnecessary cast to QRectF on GraphicsContext::fillRoundedRect.

[WebKit Review Bot]

Comment on attachment 149934 [details]
Proposed patch (v3)

Clearing flags on attachment: 149934

Committed r121435: <http://trac.webkit.org/changeset/121435>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Bruno Abinader (history only)]

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.

[Bruno Abinader (history only)]

Changing status to "reopened" while additional patch is pending review/commit-queue authorization.

[WebKit Review Bot]

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>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Noam Rosenthal]

I believe this broke the Qt Linux release bot. Mind taking a look?

[Noam Rosenthal]

Reopening to attach new patch.

[Noam Rosenthal]

Created attachment 150271 [details]
Patch for landing

[Bruno Abinader (history only)]

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?

[WebKit Review Bot]

Comment on attachment 150271 [details]
Patch for landing

Clearing flags on attachment: 150271

Committed r121618: <http://trac.webkit.org/changeset/121618>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Bruno Abinader (history only)]

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.

[Bruno Abinader (history only)]

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.

[Bruno Abinader (history only)]

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.

[Allan Sandfeld Jensen]

The fix was rolled out, so the issue is still open.

[Lamarque V. Souza]

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.

[Allan Sandfeld Jensen]

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?

[Lamarque V. Souza]

(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.

[Lamarque V. Souza]

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.

[Allan Sandfeld Jensen]

Created attachment 192473 [details]
Patch

[Allan Sandfeld Jensen]

(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.

[Allan Sandfeld Jensen]

Committed r145810: <http://trac.webkit.org/changeset/145810>