Bug 74982

Summary: [chromium] Estimate pixel count for frame rate control
Product: WebKit Reporter: Shawn Singh <shawnsingh>
Component: Layout and RenderingAssignee: W. James MacLean <wjmaclean>
Status: RESOLVED FIXED    
Severity: Normal CC: backer, cc-bugs, danakj, dglazkov, jamesr, nduca, webkit.review.bot, wjmaclean
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
not for review, just for prototyping
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing none

Description Shawn Singh 2011-12-20 17:31:11 PST
The first version is purely experimental, just for the sake of prototyping some adaptive frame rate control.  We should definitely discuss the design of the "real" version after getting an idea of how the pieces will fit together.  And of course tests should be added to make sure these estimates are computed correctly.

Just for kicks I also estimated average overdraw per pixel, too.  It may be worth experimenting with them for frame rate control.
Comment 1 Shawn Singh 2011-12-20 17:42:48 PST
Created attachment 120125 [details]
not for review, just for prototyping

This should work well enough to use, but lets discuss how we should implement it cleanly when you have time
Comment 2 Nat Duca 2012-01-09 11:48:51 PST
Shawn, any update on getting this reviewed, landed, with a histogram added as well?

Dana, James, FYI.
Comment 3 Shawn Singh 2012-01-09 15:32:18 PST
(In reply to comment #2)
> Shawn, any update on getting this reviewed, landed, with a histogram added as well?
> 
> Dana, James, FYI.

I believe James had already written a revision patch with histogram.  I think its appropriate if he can submit it here, and James and I can discuss it before asking others to review it.

Sound OK to you, James?
Comment 4 W. James MacLean 2012-01-09 15:33:21 PST
(In reply to comment #3)
> (In reply to comment #2)
> > Shawn, any update on getting this reviewed, landed, with a histogram added as well?
> > 
> > Dana, James, FYI.
> 
> I believe James had already written a revision patch with histogram.  I think its appropriate if he can submit it here, and James and I can discuss it before asking others to review it.
> 
> Sound OK to you, James?

Sure ... I'll garden it, tidy it up and post it ... stay tuned.
Comment 5 W. James MacLean 2012-01-10 15:59:02 PST
Created attachment 121929 [details]
Patch
Comment 6 Dana Jansens 2012-01-10 16:05:14 PST
Comment on attachment 121929 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:724
> +static float quadArea(const FloatQuad& quad)

Should the function name say something about the scaling to viewport?
Comment 7 W. James MacLean 2012-01-10 16:07:26 PST
Comment on attachment 121929 [details]
Patch

Again, just ideas for discussion.

This patch places the "pixels drawn" measure into LRC's drawTexturedQuad (which pretty much all drawing seems to go through), and compensates for the draw transform. It can be extended to include scissoring by intersecting the transformed quad with the scissor rect, and taking the area of that (the quadArea function can be trivially extended to compute the area of any planar polygon - computing the intersection polygon is harder but not unreasonable).

Output is in fraction of viewport drawn (1.0 = every pixel drawn once). To see results * 1000, enter "about:histograms" in the location bar, and search for "pixelOverDraw".

Does not estimate how much overdraw is "unavoidable overdraw", e.g. a fully transparent layer over a fully opaque one.
Comment 8 W. James MacLean 2012-01-10 16:12:02 PST
(In reply to comment #6)
> (From update of attachment 121929 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121929&action=review
> 
> > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:724
> > +static float quadArea(const FloatQuad& quad)
> 
> Should the function name say something about the scaling to viewport?

Sure, although the fact it's outputting quad area as a fraction of the viewport size is mostly dependent on the scaling of the input quad, except for the factor 0.25 I added (0.125 = 0.5 * 0.25, where the 0.5 should always be included) ... it might be better to move that factor somewhere else out of the function ...
Comment 9 Nat Duca 2012-01-10 17:38:51 PST
Comment on attachment 121929 [details]
Patch

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

We should also support tracking the pre-culling pixels touched.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:695
> +    PlatformSupport::histogramCustomCounts("Renderer4.pixelOverDraw",

I would rather this was done outside LayerRendererChromium. Perhaps in CLTHI.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:770
> +    m_pixelsDrawn += quadArea(drawTransform.mapQuad(quad));

Can we have this on a struct like
DrawingStatistics {
  pixelsDrawn;
}
and pass that around? I dont like ephemeral state on LRC.
Comment 10 W. James MacLean 2012-02-13 07:38:23 PST
Created attachment 126770 [details]
Patch
Comment 11 WebKit Review Bot 2012-02-13 07:55:21 PST
Comment on attachment 126770 [details]
Patch

Attachment 126770 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11506817
Comment 12 W. James MacLean 2012-02-13 08:11:48 PST
Created attachment 126774 [details]
Patch
Comment 13 Nat Duca 2012-02-16 22:14:19 PST
Comment on attachment 126774 [details]
Patch

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

What should we have for unit tests here? Presumably we can build on the quad tests to do stuff like, "for geometry x, we expect to see overdraw of blahblahblah."

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

Remove this line.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:246
> +    // FIXME: Only compute overdraw metrics occasionally, not on every frame.

How heavy is computing the metrics, in your estimation?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:251
> +    TRACE_EVENT1("CCLayerTreeHostImpl::optimizeRenderPasses", "pixelOverdraw", "opaque", static_cast<int>(normalization * overdrawCounts.m_drawnOpaque));

I think you want histograms here, not trace_events. git grep in the cc folder for histogram for the right code.

> Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.h:42
> +    float m_drawnOpaque;

Add //s for each of these explaining what they represent.

Adjust the variable name to show unit.

> Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.h:55
> +    void optimizeQuads(CCOverdrawCounts*);

can we pass in 0 to avoid the calulation? Might make a note in a // to point out that this is valid.
Comment 14 W. James MacLean 2012-02-17 05:41:34 PST
(In reply to comment #13)
> (From update of attachment 126774 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=126774&action=review
> 
> What should we have for unit tests here? Presumably we can build on the quad tests to do stuff like, "for geometry x, we expect to see overdraw of blahblahblah."
> 
> > Source/WebCore/ChangeLog:8
> > +        No new tests. (OOPS!)
> 
> Remove this line.

OK, although I had thought we might need to add a test.

> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:246
> > +    // FIXME: Only compute overdraw metrics occasionally, not on every frame.
> 
> How heavy is computing the metrics, in your estimation?

I don't have a great answer, although I suspect it's heavier that we want to do per frame. It's done per-quad, and in the case of a quad that is drawn involves at least one extra transform applied to map a quad, sometimes two (but no inverses), and then multiple invocations of the area function (pretty cheap). If the transforms and intersections aren't considered heavy, then it's not too bad.

I'll do a simple experiment to see how expensive it is ...

> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:251
> > +    TRACE_EVENT1("CCLayerTreeHostImpl::optimizeRenderPasses", "pixelOverdraw", "opaque", static_cast<int>(normalization * overdrawCounts.m_drawnOpaque));
> 
> I think you want histograms here, not trace_events. git grep in the cc folder for histogram for the right code.

Oh. That's what I originally had (see patch https://bugs.webkit.org/attachment.cgi?id=121929), but I thought you had wanted TRACE events, so I changed. Will change back.

> > Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.h:42
> > +    float m_drawnOpaque;
> 
> Add //s for each of these explaining what they represent.

Ok.

> Adjust the variable name to show unit.

All are pixels, although histogram values reported are unitless (and scaled to 1).

> > Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.h:55
> > +    void optimizeQuads(CCOverdrawCounts*);
> 
> can we pass in 0 to avoid the calulation? Might make a note in a // to point out that this is valid.

Yes, we can - I'll add a note.
Comment 15 W. James MacLean 2012-02-17 08:16:45 PST
Created attachment 127592 [details]
Patch
Comment 16 W. James MacLean 2012-02-17 08:23:06 PST
(In reply to comment #14)
> 
> I'll do a simple experiment to see how expensive it is ...

So assuming monotonicallyIncreasingTime() has sufficient resolution, I collected culling times with and without overdraw calculation, for alternating frames, for an Aura build with a single chrome window on the Aura desktop. I used alternating frames to try and keep the culling samples fairly similar between the two populations.

with Overdraw calculation:

average time for culling: 1.14 mS
std dev for culling: 0.74

without Overdraw calculation:

average time for culling: 1.05 mS
std dev for culling: 0.74 mS
  
It's a fairly small sample size, but then the computations should be fairly deterministic, so the overdraw computation adds (roughly) 8.7% to the culling time (both scale with the number of quads in the render pass)

> All are pixels, although histogram values reported are unitless (and scaled to 1).

Sorry, scaled to 1000, I mispoke.
Comment 17 Nat Duca 2012-02-27 13:44:07 PST
(In reply to comment #16)

I think you should make 2 or 3 tests that create some quads and verify that the values this creates are correct.

If you some tests created, then it LGTM and bounce it over to jamesr for review.
Comment 18 W. James MacLean 2012-02-27 14:30:07 PST
(In reply to comment #17)
> (In reply to comment #16)
> 
> I think you should make 2 or 3 tests that create some quads and verify that the values this creates are correct.
> 
> If you some tests created, then it LGTM and bounce it over to jamesr for review.

Thanks NAt! I was holding off on implementing tests until I got some feedback on the basic approach.
Comment 19 W. James MacLean 2012-02-28 07:29:34 PST
Created attachment 129250 [details]
Patch
Comment 20 Adrienne Walker 2012-02-29 12:14:43 PST
Comment on attachment 129250 [details]
Patch

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

Looks good to me!

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:244
> +    float normalization = 1000.f / (m_layerRenderer->viewportWidth() * m_layerRenderer->viewportHeight());

Out of curiosity, what's the 1000 for? Is there some limitation on the values that the histograms can capture?
Comment 21 James Robinson 2012-02-29 12:22:46 PST
Comment on attachment 129250 [details]
Patch

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

R=me, looks good.

Do we have a good sense for the cost of this? I think it should be pretty low, right?

> Source/WebCore/platform/graphics/chromium/cc/CCQuadCuller.cpp:133
> +        if (overdrawMetrics) {

can you add a TRACE_EVENT for this block so we can tell if the overdraw calculation is getting heavy when looking at traces?
Comment 22 James Robinson 2012-02-29 12:23:34 PST
(In reply to comment #20)
> (From update of attachment 129250 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=129250&action=review
> 
> Looks good to me!
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:244
> > +    float normalization = 1000.f / (m_layerRenderer->viewportWidth() * m_layerRenderer->viewportHeight());
> 
> Out of curiosity, what's the 1000 for? Is there some limitation on the values that the histograms can capture?

This is a 'counts' histogram, so we need to get integers out at the end. I think the idea here is to map to the range [0, 1000]
Comment 23 W. James MacLean 2012-02-29 12:26:14 PST
(In reply to comment #21)
> (From update of attachment 129250 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=129250&action=review
> 
> R=me, looks good.
> 
> Do we have a good sense for the cost of this? I think it should be pretty low, right?

Yes, should be less than 10% of the culling cost, and we can ultimately decide to just run it on selected frames.
 
> > Source/WebCore/platform/graphics/chromium/cc/CCQuadCuller.cpp:133
> > +        if (overdrawMetrics) {
> 
> can you add a TRACE_EVENT for this block so we can tell if the overdraw calculation is getting heavy when looking at traces?

Good idea, consider it done!
Comment 24 W. James MacLean 2012-02-29 12:26:29 PST
(In reply to comment #22)
> (In reply to comment #20)
> > (From update of attachment 129250 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=129250&action=review
> > 
> > Looks good to me!
> > 
> > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:244
> > > +    float normalization = 1000.f / (m_layerRenderer->viewportWidth() * m_layerRenderer->viewportHeight());
> > 
> > Out of curiosity, what's the 1000 for? Is there some limitation on the values that the histograms can capture?
> 
> This is a 'counts' histogram, so we need to get integers out at the end. I think the idea here is to map to the range [0, 1000]

Yup!
Comment 25 W. James MacLean 2012-02-29 12:27:56 PST
(In reply to comment #22)
> (In reply to comment #20)
> > (From update of attachment 129250 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=129250&action=review
> > 
> > Looks good to me!
> > 
> > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:244
> > > +    float normalization = 1000.f / (m_layerRenderer->viewportWidth() * m_layerRenderer->viewportHeight());
> > 
> > Out of curiosity, what's the 1000 for? Is there some limitation on the values that the histograms can capture?
> 
> This is a 'counts' histogram, so we need to get integers out at the end. I think the idea here is to map to the range [0, 1000]

Actually, 1000 <--> 1.0, where 1.0 => zero overdraw. In reality we expect the values to be somewhat above 1.0 as we can't prevent all overdraw, but we don't want it being 2.0 (2000) either.
Comment 26 W. James MacLean 2012-02-29 12:51:17 PST
Created attachment 129493 [details]
Patch for landing
Comment 27 WebKit Review Bot 2012-02-29 19:18:42 PST
Comment on attachment 129493 [details]
Patch for landing

Rejecting attachment 129493 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
5893f545307eee0e0891d77ce2b1c1
r109289 = 76edf1862c2775c8a612a5c1ae46efdfd7eef552
r109290 = 29f8c8bcf28f90ab977260a3e046d2e3199160be
r109291 = 8342c092acf17bb8b669c009b37c09dc223a07e9
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
RA layer request failed: OPTIONS of 'http://svn.webkit.org/repository/webkit': timed out waiting for server (http://svn.webkit.org) at /usr/lib/git-core/git-svn line 2295

Died at Tools/Scripts/update-webkit line 164.

Full output: http://queues.webkit.org/results/11776115
Comment 28 Adam Barth 2012-02-29 19:40:57 PST
Comment on attachment 129493 [details]
Patch for landing

Sorry, the SVN outage is confusing the bots.
Comment 29 WebKit Review Bot 2012-02-29 20:33:50 PST
Comment on attachment 129493 [details]
Patch for landing

Clearing flags on attachment: 129493

Committed r109304: <http://trac.webkit.org/changeset/109304>
Comment 30 WebKit Review Bot 2012-02-29 20:33:57 PST
All reviewed patches have been landed.  Closing bug.