| Summary: | [chromium] Estimate pixel count for frame rate control | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Shawn Singh <shawnsingh> | ||||||||||||||||
| Component: | Layout and Rendering | Assignee: | 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
Shawn Singh
2011-12-20 17:31:11 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
Shawn, any update on getting this reviewed, landed, with a histogram added as well? Dana, James, FYI. (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? (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. Created attachment 121929 [details]
Patch
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 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.
(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 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. Created attachment 126770 [details]
Patch
Comment on attachment 126770 [details] Patch Attachment 126770 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11506817 Created attachment 126774 [details]
Patch
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. (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. Created attachment 127592 [details]
Patch
(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. (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. (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. Created attachment 129250 [details]
Patch
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 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? (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] (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! (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! (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. Created attachment 129493 [details]
Patch for landing
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 on attachment 129493 [details]
Patch for landing
Sorry, the SVN outage is confusing the bots.
Comment on attachment 129493 [details] Patch for landing Clearing flags on attachment: 129493 Committed r109304: <http://trac.webkit.org/changeset/109304> All reviewed patches have been landed. Closing bug. |