Optimize box-shadow rendering even further by doing more intersections

RESOLVED FIXED in mozilla1.9.2a1

Status

()

Core
Graphics
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: Michael Ventnor, Assigned: Michael Ventnor)

Tracking

({fixed1.9.1, perf})

Trunk
mozilla1.9.2a1
fixed1.9.1, perf
Points:
---
Bug Flags:
wanted1.9.1 ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

9 years ago
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.
Attachment #351495 - Flags: superreview?(roc)
Attachment #351495 - Flags: review?(roc)
+                     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.
(Assignee)

Comment 2

9 years ago
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.
Attachment #351495 - Attachment is obsolete: true
Attachment #351499 - Flags: review?(vladimir)
Attachment #351495 - Flags: superreview?(roc)
Attachment #351495 - Flags: review?(roc)
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.
Attachment #351499 - Flags: review?(vladimir) → review+
(Assignee)

Comment 4

9 years ago
Created attachment 351658 [details] [diff] [review]
Patch 3
[Checkin: Comment 5]

Yeah, that's right.
Attachment #351499 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
Whiteboard: [needs landing]
Comment on attachment 351658 [details] [diff] [review]
Patch 3
[Checkin: Comment 5]

http://hg.mozilla.org/mozilla-central/rev/c333209e9650
Attachment #351658 - Attachment description: Patch 3 → Patch 3 [Checkin: Comment 5]
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing]
Target Milestone: --- → mozilla1.9.2a1

Comment 6

9 years ago
Can this be also checked into the 1.9.1 branch?
It would be nice to start using box-shadow without this performance penalty.

Updated

9 years ago
Depends on: 458031
Flags: wanted1.9.1?
Keywords: perf
OS: Linux → All
Hardware: PC → All
(Assignee)

Comment 7

8 years ago
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.
Attachment #351658 - Flags: approval1.9.1?
Comment on attachment 351658 [details] [diff] [review]
Patch 3
[Checkin: Comment 5]

a191=beltzner
Attachment #351658 - Flags: approval1.9.1? → approval1.9.1+
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
Whiteboard: [needs 191 landing]

Comment 9

8 years ago
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?
(Assignee)

Comment 10

8 years ago
That check happens at a higher level.
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
}
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
Whiteboard: [needs 191 landing]
(Assignee)

Updated

8 years ago
Whiteboard: [needs 191 landing]
Pushed 3136ba367137 to 1.9.1
Keywords: fixed1.9.1
Whiteboard: [needs 191 landing]
You need to log in before you can comment on or make changes to this bug.