Bug 74982 – [chromium] Estimate pixel count for frame rate control

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Shawn Singh]

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.

[Shawn Singh]

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

[Nat Duca]

Shawn, any update on getting this reviewed, landed, with a histogram added as well?

Dana, James, FYI.

[Shawn Singh]

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

[W. James MacLean]

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

[W. James MacLean]

Created attachment 121929 [details]
Patch

[Dana Jansens]

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?

[W. James MacLean]

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.

[W. James MacLean]

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

[Nat Duca]

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.

[W. James MacLean]

Created attachment 126770 [details]
Patch

[WebKit Review Bot]

Comment on attachment 126770 [details]
Patch

Attachment 126770 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11506817

[W. James MacLean]

Created attachment 126774 [details]
Patch

[Nat Duca]

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.

[W. James MacLean]

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

[W. James MacLean]

Created attachment 127592 [details]
Patch

[W. James MacLean]

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

[Nat Duca]

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

[W. James MacLean]

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

[W. James MacLean]

Created attachment 129250 [details]
Patch

[Adrienne Walker]

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?

[James Robinson]

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?

[James Robinson]

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

[W. James MacLean]

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

[W. James MacLean]

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

[W. James MacLean]

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

[W. James MacLean]

Created attachment 129493 [details]
Patch for landing

[WebKit Review Bot]

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

[Adam Barth]

Comment on attachment 129493 [details]
Patch for landing

Sorry, the SVN outage is confusing the bots.

[WebKit Review Bot]

Comment on attachment 129493 [details]
Patch for landing

Clearing flags on attachment: 129493

Committed r109304: <http://trac.webkit.org/changeset/109304>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.