Last Comment Bug 468018 - Optimize box-shadow rendering even further by doing more intersections
: Optimize box-shadow rendering even further by doing more intersections
Status: RESOLVED FIXED
: fixed1.9.1, perf
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: All All
-- normal (vote)
: mozilla1.9.2a1
Assigned To: Michael Ventnor
:
: Milan Sreckovic [:milan]
Mentors:
Depends on: 458031
Blocks:
  Show dependency treegraph
 
Reported: 2008-12-04 19:24 PST by Michael Ventnor
Modified: 2009-01-08 18:10 PST (History)
5 users (show)
hskupin: wanted1.9.1?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (4.68 KB, patch)
2008-12-04 19:24 PST, Michael Ventnor
no flags Details | Diff | Splinter Review
Patch 2 (5.29 KB, patch)
2008-12-04 19:56 PST, Michael Ventnor
vladimir: review+
Details | Diff | Splinter Review
Patch 3 [Checkin: Comment 5] (5.31 KB, patch)
2008-12-05 21:22 PST, Michael Ventnor
mbeltzner: approval1.9.1+
Details | Diff | Splinter Review

Description User image Michael Ventnor 2008-12-04 19:24:44 PST
Created attachment 351495 [details] [diff] [review]
Patch 

I've actually figured out an easy way to fix the XXX comment that we added in bug 458031 and still keep suitable performance for HTML canvas use by avoiding these optimizations where they are not needed. It also doesn't break the current API. Whether it passes review is another story but at the moment this is the only suitable approach that has come to mind.
Comment 1 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2008-12-04 19:42:21 PST
+                     const gfxRect* aDirtyRect = nsnull);

Can you not use a default parameter here? Just modify the other caller to pass nsnull.

Make mDirtyRect be empty to indicate there's no dirty rect is very confusing. You should probably just have a separate flag to indicate if mDirtyRect should be used.

Otherwise looks good to me, but you should get your next review from vlad.
Comment 2 User image Michael Ventnor 2008-12-04 19:56:31 PST
Created attachment 351499 [details] [diff] [review]
Patch 2

Sure. The only reason I did the default parameter was because I thought there were more calls in canvas code than there were.

This patch does not change canvas's behaviour at all, only box-shadow/text-shadow's. Tests pass.
Comment 3 User image Vladimir Vukicevic [:vlad] [:vladv] 2008-12-05 13:48:15 PST
Comment on attachment 351499 [details] [diff] [review]
Patch 2

I don't like the variable name "requiredShadowArea" -- that should be "requiredBlurArea", no?  Looks fine otherwise.
Comment 4 User image Michael Ventnor 2008-12-05 21:22:54 PST
Created attachment 351658 [details] [diff] [review]
Patch 3
[Checkin: Comment 5]

Yeah, that's right.
Comment 5 User image Serge Gautherie (:sgautherie) 2008-12-06 11:04:40 PST
Comment on attachment 351658 [details] [diff] [review]
Patch 3
[Checkin: Comment 5]

http://hg.mozilla.org/mozilla-central/rev/c333209e9650
Comment 6 User image Alfred Kayser 2008-12-07 04:55:58 PST
Can this be also checked into the 1.9.1 branch?
It would be nice to start using box-shadow without this performance penalty.
Comment 7 User image Michael Ventnor 2009-01-04 17:54:32 PST
Comment on attachment 351658 [details] [diff] [review]
Patch 3
[Checkin: Comment 5]

This patch has baked for a while on trunk with no problems. Sure, let's get this on 1.9.1 considering we'd like to encourage people to try out more CSS3.
Comment 8 User image Mike Beltzner [:beltzner, not reading bugmail] 2009-01-05 22:32:51 PST
Comment on attachment 351658 [details] [diff] [review]
Patch 3
[Checkin: Comment 5]

a191=beltzner
Comment 9 User image Alfred Kayser 2009-01-06 00:58:14 PST
For the following lines in gfxBlur:
    if (rect.IsEmpty())
        return nsnull;
    if (aDirtyRect) {
...
        rect = requiredBlurArea.Intersect(rect);

Could it be that with a aDirtyRect specified, the rect can be empty after the Insect with aDirtyRect?
If so, after that Intersect, one should/could also return nsnull, as there is nothing to draw?
Comment 10 User image Michael Ventnor 2009-01-06 01:18:14 PST
That check happens at a higher level.
Comment 11 User image Serge Gautherie (:sgautherie) 2009-01-06 03:58:19 PST
Comment on attachment 351658 [details] [diff] [review]
Patch 3
[Checkin: Comment 5]

{
patching file gfx/thebes/src/gfxBlur.cpp
Hunk #2 FAILED at 221
1 out of 2 hunks FAILED
}
Comment 12 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2009-01-08 18:10:17 PST
Pushed 3136ba367137 to 1.9.1

Note You need to log in before you can comment on or make changes to this bug.