Bug 81242 - [chromium] Remove surface damage client from occlusion tracker
Summary: [chromium] Remove surface damage client from occlusion tracker
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dana Jansens
URL:
Keywords:
Depends on: 81222
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-15 11:22 PDT by Dana Jansens
Modified: 2012-03-16 10:00 PDT (History)
7 users (show)

See Also:


Attachments
Patch (15.03 KB, patch)
2012-03-15 11:32 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (15.15 KB, patch)
2012-03-15 12:06 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (15.31 KB, patch)
2012-03-16 07:30 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dana Jansens 2012-03-15 11:22:08 PDT
[chromium] Remove surface damage client from occlusion tracker
Comment 1 Dana Jansens 2012-03-15 11:23:28 PDT
I claim that the root surface damage rect will cull at least as well as the (bounding box of the) rect transformed into the current target surface.

Proof by picture http://imgur.com/r5I1s
Comment 2 Dana Jansens 2012-03-15 11:32:24 PDT
Created attachment 132085 [details]
Patch
Comment 3 Shawn Singh 2012-03-15 11:59:11 PDT
Comment on attachment 132085 [details]
Patch


Looks great... but I have to admit I'm not wrapping my brain fully around the proof of equivalence yet.  But if someone else already feels confident about it, then that's good enough for me =)  Otherwise I'll take a closer look later tonight.


View in context: https://bugs.webkit.org/attachment.cgi?id=132085&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.cpp:321
>  {

Maybe we should now rename this function and the variables inside it, since it is not really a "scissor" anymore.  Its just the clipRect, right?

> Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.cpp:324
>      if (!layer->clipRect().isEmpty())

Since we're driving by, could we add a FIXME referring to the usesLayerClipping bug?  https://bugs.webkit.org/show_bug.cgi?id=80622
Comment 4 Dana Jansens 2012-03-15 12:05:16 PDT
(In reply to comment #3)
> (From update of attachment 132085 [details])
> 
> Looks great... but I have to admit I'm not wrapping my brain fully around the proof of equivalence yet.  But if someone else already feels confident about it, then that's good enough for me =)  Otherwise I'll take a closer look later tonight.
> 
> 
> View in context: https://bugs.webkit.org/attachment.cgi?id=132085&action=review
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.cpp:321
> >  {
> 
> Maybe we should now rename this function and the variables inside it, since it is not really a "scissor" anymore.  Its just the clipRect, right?

Well, it's the clip rect + the target's content bounds. Which is sort of like the visibleLayerRect.. but in this case acts like a scissor. I think the name still makes sense under those terms?

> > Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.cpp:324
> >      if (!layer->clipRect().isEmpty())
> 
> Since we're driving by, could we add a FIXME referring to the usesLayerClipping bug?  https://bugs.webkit.org/show_bug.cgi?id=80622

Sure!
Comment 5 Dana Jansens 2012-03-15 12:06:29 PDT
Created attachment 132097 [details]
Patch
Comment 6 Adrienne Walker 2012-03-15 18:23:19 PDT
Comment on attachment 132097 [details]
Patch

I think I'm convinced that scissoring in screen space is always going to cull just as much (if not more) than mapping the scissor onto a layer and taking the enclosing IntRect and scissoring with that.  Thanks for the diagram.

It's nice not to have to need this client class.  R=me.
Comment 7 Dana Jansens 2012-03-16 07:30:51 PDT
Created attachment 132279 [details]
Patch
Comment 8 WebKit Review Bot 2012-03-16 09:59:57 PDT
Comment on attachment 132279 [details]
Patch

Clearing flags on attachment: 132279

Committed r111021: <http://trac.webkit.org/changeset/111021>
Comment 9 WebKit Review Bot 2012-03-16 10:00:03 PDT
All reviewed patches have been landed.  Closing bug.