Bug 79400

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

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?

(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! =)

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

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

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

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.

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

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

(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)?

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

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.

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

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

Created attachment 138152 [details] Addressed reviewer comments

Comment on attachment 138152 [details] Addressed reviewer comments Awesome, Shawn. LGTM.

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?

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

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; }

I was suggesting that you let the GPU handle the transformation and clipping. What are the inspector dependencies here?

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

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.

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.

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

Created attachment 138385 [details] Patch for landing

Comment on attachment 138385 [details] Patch for landing Clearing flags on attachment: 138385 Committed r114963: <http://trac.webkit.org/changeset/114963>

All reviewed patches have been landed. Closing bug.