Bug 79400 – [chromium] Visualize accelerated compositor update/draw/damage rects.

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Shawn Singh]

This patch is probably never going to land, but it might be useful for other people who may want to estimate where performance is being spent.  It has been extremely useful for getting damage tracking implemented properly.

(1) updateRects (red): rects that came from dirty regions of layers.  The updateRect is the region of a layer that was actually updated on the texture resource.  i.e. these portions of layers may have affected the screen.
(2) drawRect (blue):  rects from layers whose properties have changed, i.e. the layer's old position and new position affect the screen
(3) exposedRects (purple):  currently not visualized on this patch, let me know if you need it.  i.e. the layer's old position that affects the screen after it disappears or moves.
(4) damageRects (orange/dirty-yellow): the damage rect for each surface.  the root-level rect is the one that would be a scissor.

caveats:
  does not consider tiles / quads / culling.

Note that updateRects, drawRects, and exposed rects are unrelated.  draw rects do NOT include updateRects. -- all three types of rects contribute to the final damage.

I'm not planning to maintain this except when its convenient or urgently needed.  But, if enough people want this to actually land (after addressing a few issues) then let's discuss it =)

[Shawn Singh]

Created attachment 128557 [details]
Debug visualization, not for review

[Nat Duca]

I definitely support landing a simplified version of this patch  (but lets fix the HUD while we're at it, its not hard).

I do want to ask a probing question though: should we simplify what we display a bit? This is a very developer-friendly and developer-evangelizable feature, but the updateRects are the only real item for the developer can control, right?

[Shawn Singh]

(In reply to comment #2)
> I definitely support landing a simplified version of this patch  (but lets fix the HUD while we're at it, its not hard).
> 
> I do want to ask a probing question though: should we simplify what we display a bit? This is a very developer-friendly and developer-evangelizable feature, but the updateRects are the only real item for the developer can control, right?

For developers, draw rects would be just as important. For example if the dev wants to do something clever with CSS (poster circle is a good example).

I think the most developer-friendly thing to do would be to make all rects available, each one under a separate flag (total 4 flags: update/draw/exposed/damage)  Is that approach OK with you?

advantages of this approach:
  - easy to simplify what's visualized
  - describing all these types of rects serves a small educational value too, to developers who care enough

Are you concerned about this being burdensome to maintain?  I don't imagine that making 3-4 flags versus only 1 or 2 will be too much harder to maintain; please give me a reality-check if you disagree! =)

[Nat Duca]

> For developers, draw rects would be just as important. For example if the dev wants to do something clever with CSS (poster circle is a good example).

I think you're vastly overestimating the ability of developers to understand the distinction between drawing and painting. As a general rule, people don't understand the whole compositing architecture, and really shouldn't have to.

> I think the most developer-friendly thing to do would be to make all rects available, each one under a separate flag (total 4 flags: update/draw/exposed/damage)  Is that approach OK with you?

No, I really think thats far too much to understand to a developer.

> Are you concerned about this being burdensome to maintain? 

No.

I'm concerned about there being a valid and commonplace optimization scenario that is solved by the flags. The vast majority of our developers are just writing CSS and HTML. The most common issue encountered is something that caused painting -- which then turns into a slew of uploads/etc. 

I think we should focus on solving this use case well for our developers. If we can get that solved, and we start seeing new use cases where large numbers of developers are having performance problems that stem from other type of rects, that is the correct time to add more options.

[Shawn Singh]

Created attachment 129726 [details]
updated to tip of tree, but not for review yet

[Nat Duca]

Lets meet quickly while @caseq is is here to figure out how to drive this into inspector.

[Shawn Singh]

I'd like to use this patch as an opportunity to refactor the fps calculation as well... so "history" and "debug info" are logged separately, and the HUD's purpose is purely to visualize information.  Let me know if that's OK:

here's what I had in mind:

class CCFPSDebugHistory
  updateFPSCounter()
  drawFPSCounter() // or something cleaner so that it can draw the HUD sparkle line

class CCRectDebugHistory
  logUpdateRects
  logDrawRects
  logExposedRects // might not be worth doing.
  logDamageRects
  drawUpdateRects
   etc...


These classes will be owned by whoever owns the HUD.  For now, they would only be used by the HUD, but this will make it cleaner for other places (like web inspector, via CCProxy) to access this information, too.

[Andrey Kosyakov]

(In reply to comment #7)

> For now, they would only be used by the HUD, but this will make it cleaner for > other places (like web inspector, via CCProxy) to access this information, too.

Inspector can't depend on the platform, so to be able to hand over the sets of rects to the inspector you would have to use common WebCore/WTF types.

[Shawn Singh]

(In reply to comment #8)
> (In reply to comment #7)
> 
> > For now, they would only be used by the HUD, but this will make it cleaner for > other places (like web inspector, via CCProxy) to access this information, too.
> 
> Inspector can't depend on the platform, so to be able to hand over the sets of rects to the inspector you would have to use common WebCore/WTF types.

Good point;  The proposed refactor would still work;  My intention was just to clean up how the compositor partitions this logic internally.

[Nat Duca]

(In reply to comment #7)
> I'd like to use this patch as an opportunity to refactor the fps calculation as well... so "history" and "debug info" are logged separately, and the HUD's purpose is purely to visualize information.  Let me know if that's OK:
> 
> here's what I had in mind:
> 
> class CCFPSDebugHistory
>   updateFPSCounter()
>   drawFPSCounter() // or something cleaner so that it can draw the HUD sparkle line
> 
> class CCRectDebugHistory
>   logUpdateRects
>   logDrawRects
>   logExposedRects // might not be worth doing.
>   logDamageRects
>   drawUpdateRects
>    etc...
> 
> 

Yes please, though maybe logRect(rect_type, rect)?

[Nat Duca]

(In reply to comment #10)
> 
> Yes please, though maybe logRect(rect_type, rect)?
And addRect(), CCDebugRects.add, and have CCLayerTreeHost.resetDebugRects(), and pass debug rects into HUD? <insert the discussion you and i had back in the day about resetting the damage tracker>

[Shawn Singh]

Created attachment 138028 [details]
Patch

(1) added 3 flags to CCSettings, disabled by default. (2) will plumb --show-paint-rects to ONLY the paintRects in a separate patch.

[Shawn Singh]

(In reply to comment #12)
> Created an attachment (id=138028) [details]
> Patch
> 
> (1) added 3 flags to CCSettings, disabled by default. (2) will plumb --show-paint-rects to ONLY the paintRects in a separate patch.

Nat,  I might have accidentally ignored your comments in Comment 10 and 11.  I'm really sorry about that!

Can you please take a quick look at CCDebugRectHistory.h on this patch, and see if that satisfies you?  Everything is squished into one public function.  Feel free to scream at me if that doesn't sit well with you.

[Nat Duca]

Comment on attachment 138028 [details]
Patch

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

No screaming required! :) This is really slick, awesome Shawn.

> Source/WebCore/platform/graphics/chromium/cc/CCDebugRectHistory.cpp:54
> +    // Note: this logging must happen before layer's property change tracking is reset.

Seems like this should be a note on the .h explaining when it should be used?

> Source/WebCore/platform/graphics/chromium/cc/CCDebugRectHistory.h:55
> +    const Vector<FloatRect>& paintRects() { return m_paintRectHistory; }

Might you save yourself some typing/code duplication and do...

struct DebugRect {
  RectType type; 
  FloatRect rect;
}

Where rectType is an enum?

> Source/WebCore/platform/graphics/chromium/cc/CCFpsCounter.h:1
> +/*

CCFpsCounter -> CCFrameRateCounter?

Quick git trick that might help this go faster:
git diff <upstream> > /tmp/patch
<edit /tmp/patch in whatever editor you like and replace CCFpsCounter everywhere>
git reset --hard upstream
patch -p1 </tmp/patch

[Shawn Singh]

Created attachment 138152 [details]
Addressed reviewer comments

[Nat Duca]

Comment on attachment 138152 [details]
Addressed reviewer comments

Awesome, Shawn. LGTM.

[Adrienne Walker]

Comment on attachment 138152 [details]
Addressed reviewer comments

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

> Source/WebCore/ChangeLog:23
> +        In addition to adding support for visualizing this, the
> +        CCHeadsUpDisplay was significantly refactored, separating the FPS
> +        Counter functionality into a different class, so that the heads-up
> +        display is all about visualizing annotations, and those
> +        annotations (frame rate, debug rects) are logged separately.

Next time, can you separate refactoring of existing classes out into a different change so it can be reviewed more easily on its own?

> Source/WebCore/platform/graphics/chromium/cc/CCDebugRectHistory.cpp:70
> +        m_debugRects.append(CCDebugRect(PaintRectType, CCMathUtil::mapClippedRect(layer->screenSpaceTransform(), layer->updateRect())));

Am I reading this right in that everything gets mapped to an axis-aligned rect in screen space? Is it possible to draw solid color quads with that transform so that you can visualize the actual rects when there's some skew or perspective involved?

[Nat Duca]

(In reply to comment #17)
> (From update of attachment 138152 [details])
> Am I reading this right in that everything gets mapped to an axis-aligned rect in screen space? Is it possible to draw solid color quads with that transform so that you can visualize the actual rects when there's some skew or perspective involved?

Oh, damn, thats a good point. Hey shawn, we should make these FloatRects in screenspace so that when we try to embed this into inspector, we don't bake the assumption of screen-aligned squares downstream from the HUD.

[Shawn Singh]

Ack! very confused now.  I think there may be problems with what you guys are asking for.  Details below, just let me know what you guys want =)


(0) Yeah, I should have separated the refactoring and this. Sorry about that.


(1) FloatRects vs FloatQuads:  I agree it would be nice to visualize quads instead of axis-aligned rects.  However, transforming FloatQuads with a perspective transform will require outputting a general polygon (because if it is clipped by w<0, it could then have 5-8 edges).  If everyone is willing to deal with the complexity associated with that, I can do it... but it will no longer be a basic WebCore data type as Andrey was requesting.

// In general this can be a clipped quad with up to 8 vertices.
// The vertices are stored in screen-space.
struct CCDebugQuad {
  FloatPoint vertices[8];
  int numVertices;
}


(2) Screen-Space vs ???:  Nat, I'm already converting everything to screen space when storing the information in the DebugRectHistory.  is that what you want?  or are you asking me to remove that so that we are "not baking the assumption upstream in the CCDebugRectHistory before giving it to the HUD/inspector"?

Doing this would essentially punt the clipped w<0 complexity to outside code, and we probably should not do that.  It would potentially create a dependency on CCMathUtil which is currently not part of WebCore.

// The rect is stored in local space, along with the transform that would convert it to screen-space.
// WARNING: when you apply the screen space transform, you will need to use CCMathUtil functions to
//          properly compute clipped transformed quads, or else you may get incorrect results for
//          some perspective transforms.
struct CCDebugQuad {
  FloatQuad (or FloatRect) localSpaceBounds;
  TransformationMatrix screenSpaceTransform;
}

[Adrienne Walker]

I was suggesting that you let the GPU handle the transformation and clipping.  

What are the inspector dependencies here?

[Adrienne Walker]

(In reply to comment #20)
> I was suggesting that you let the GPU handle the transformation and clipping.  
> 
> What are the inspector dependencies here?

Oh, I see now.  Inspector wants to draw these rects itself and so you can't rely on RLC infrastructure. Sorry for the noise.

Maybe in the future if inspector gets smart enough to draw transformed rects, we can modify this code to pass transforms and rects on through.

[Adrienne Walker]

Comment on attachment 138152 [details]
Addressed reviewer comments

This seems reasonable as a first pass.  This will overestimate 3d transformed layers, but is still useful for everything else.  R=me.

[Nat Duca]

Screenspace was the intent.

I dont think you need to store an 8gon. You can store a float quad and a clip quad if needed, both in screenspace. The overlay can simply set the clip before drawing the poly.

[Shawn Singh]

(In reply to comment #23)
> Screenspace was the intent.

OK thanks.  I'll land this after re-basing code.


> 
> I dont think you need to store an 8gon. You can store a float quad and a clip quad if needed, both in screenspace. The overlay can simply set the clip before drawing the poly.

FYI this particular issue of clipping here is not usual "clipping" in 2D or 3D - it needs to happen in homogeneous coordinates on w < 0 ... we simply can't compute a transformed quad in screenspace until we clip in homogeneous space before divide-by-w, or else it may be invalid. (see https://bugs.webkit.org/show_bug.cgi?id=80806 for more details, if you're interested)

[Shawn Singh]

Created attachment 138385 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 138385 [details]
Patch for landing

Clearing flags on attachment: 138385

Committed r114963: <http://trac.webkit.org/changeset/114963>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.